Merge pull request #2420 from github/koesie10/cancel-monitor-on-404

Cancel monitoring variant analysis on 404 response

Author: koesie10

extensions/ql-vscode/src/common/commands.ts

@@ -251,6 +251,9 @@ export type VariantAnalysisCommands = {
   "codeQL.monitorRehydratedVariantAnalysis": (
     variantAnalysis: VariantAnalysis,
   ) => Promise<void>;
+  "codeQL.monitorReauthenticatedVariantAnalysis": (
+    variantAnalysis: VariantAnalysis,
+  ) => Promise<void>;
   "codeQL.openVariantAnalysisLogs": (
     variantAnalysisId: number,
   ) => Promise<void>;

extensions/ql-vscode/src/common/vscode/authentication.ts

@@ -3,7 +3,7 @@ import * as Octokit from "@octokit/rest";
 import { retry } from "@octokit/plugin-retry";
 import { Credentials } from "../authentication";
 
-const GITHUB_AUTH_PROVIDER_ID = "github";
+export const GITHUB_AUTH_PROVIDER_ID = "github";
 
 // We need 'repo' scope for triggering workflows, 'gist' scope for exporting results to Gist,
 // and 'read:packages' for reading private CodeQL packages.

extensions/ql-vscode/src/variant-analysis/variant-analysis-manager.ts

@@ -5,6 +5,8 @@ import {
   getVariantAnalysisRepo,
 } from "./gh-api/gh-api-client";
 import {
+  authentication,
+  AuthenticationSessionsChangeEvent,
   CancellationToken,
   env,
   EventEmitter,
@@ -72,6 +74,7 @@ import {
   REPO_STATES_FILENAME,
   writeRepoStates,
 } from "./repo-states-store";
+import { GITHUB_AUTH_PROVIDER_ID } from "../common/vscode/authentication";
 
 export class VariantAnalysisManager
   extends DisposableObject
@@ -131,6 +134,10 @@ export class VariantAnalysisManager
     this.variantAnalysisResultsManager.onResultLoaded(
       this.onRepoResultLoaded.bind(this),
     );
+
+    this.push(
+      authentication.onDidChangeSessions(this.onDidChangeSessions.bind(this)),
+    );
   }
 
   getCommands(): VariantAnalysisCommands {
@@ -144,6 +151,8 @@ export class VariantAnalysisManager
         this.monitorVariantAnalysis.bind(this),
       "codeQL.monitorRehydratedVariantAnalysis":
         this.monitorVariantAnalysis.bind(this),
+      "codeQL.monitorReauthenticatedVariantAnalysis":
+        this.monitorVariantAnalysis.bind(this),
       "codeQL.openVariantAnalysisLogs": this.openVariantAnalysisLogs.bind(this),
       "codeQL.openVariantAnalysisView": this.showView.bind(this),
       "codeQL.runVariantAnalysis":
@@ -504,6 +513,38 @@ export class VariantAnalysisManager
     repoStates[repoState.repositoryId] = repoState;
   }
 
+  private async onDidChangeSessions(
+    event: AuthenticationSessionsChangeEvent,
+  ): Promise<void> {
+    if (event.provider.id !== GITHUB_AUTH_PROVIDER_ID) {
+      return;
+    }
+
+    for (const variantAnalysis of this.variantAnalyses.values()) {
+      if (
+        this.variantAnalysisMonitor.isMonitoringVariantAnalysis(
+          variantAnalysis.id,
+        )
+      ) {
+        continue;
+      }
+
+      if (
+        await isVariantAnalysisComplete(
+          variantAnalysis,
+          this.makeResultDownloadChecker(variantAnalysis),
+        )
+      ) {
+        continue;
+      }
+
+      void this.app.commands.execute(
+        "codeQL.monitorReauthenticatedVariantAnalysis",
+        variantAnalysis,
+      );
+    }
+  }
+
   public async monitorVariantAnalysis(
     variantAnalysis: VariantAnalysis,
   ): Promise<void> {

extensions/ql-vscode/src/variant-analysis/variant-analysis-monitor.ts

@@ -1,5 +1,6 @@
 import { env, EventEmitter } from "vscode";
 import { getVariantAnalysis } from "./gh-api/gh-api-client";
+import { RequestError } from "@octokit/request-error";
 
 import {
   isFinalVariantAnalysisStatus,
@@ -27,6 +28,8 @@ export class VariantAnalysisMonitor extends DisposableObject {
   );
   readonly onVariantAnalysisChange = this._onVariantAnalysisChange.event;
 
+  private readonly monitoringVariantAnalyses = new Set<number>();
+
   constructor(
     private readonly app: App,
     private readonly shouldCancelMonitor: (
@@ -36,9 +39,37 @@ export class VariantAnalysisMonitor extends DisposableObject {
     super();
   }
 
+  public isMonitoringVariantAnalysis(variantAnalysisId: number): boolean {
+    return this.monitoringVariantAnalyses.has(variantAnalysisId);
+  }
+
   public async monitorVariantAnalysis(
     variantAnalysis: VariantAnalysis,
   ): Promise<void> {
+    if (this.monitoringVariantAnalyses.has(variantAnalysis.id)) {
+      void extLogger.log(
+        `Already monitoring variant analysis ${variantAnalysis.id}`,
+      );
+      return;
+    }
+
+    this.monitoringVariantAnalyses.add(variantAnalysis.id);
+    try {
+      await this._monitorVariantAnalysis(variantAnalysis);
+    } finally {
+      this.monitoringVariantAnalyses.delete(variantAnalysis.id);
+    }
+  }
+
+  private async _monitorVariantAnalysis(
+    variantAnalysis: VariantAnalysis,
+  ): Promise<void> {
+    const variantAnalysisLabel = `${variantAnalysis.query.name} (${
+      variantAnalysis.query.language
+    }) [${new Date(variantAnalysis.executionStartTime).toLocaleString(
+      env.language,
+    )}]`;
+
     let attemptCount = 0;
     const scannedReposDownloaded: number[] = [];
 
@@ -61,11 +92,7 @@ export class VariantAnalysisMonitor extends DisposableObject {
       } catch (e) {
         const errorMessage = getErrorMessage(e);
 
-        const message = `Error while monitoring variant analysis ${
-          variantAnalysis.query.name
-        } (${variantAnalysis.query.language}) [${new Date(
-          variantAnalysis.executionStartTime,
-        ).toLocaleString(env.language)}]: ${errorMessage}`;
+        const message = `Error while monitoring variant analysis ${variantAnalysisLabel}: ${errorMessage}`;
 
         // If we have already shown this error to the user, don't show it again.
         if (lastErrorShown === errorMessage) {
@@ -75,6 +102,19 @@ export class VariantAnalysisMonitor extends DisposableObject {
           lastErrorShown = errorMessage;
         }
 
+        if (e instanceof RequestError && e.status === 404) {
+          // We want to show the error message to the user, but we don't want to
+          // keep polling for the variant analysis if it no longer exists.
+          // Therefore, this block is down here rather than at the top of the
+          // catch block.
+          void extLogger.log(
+            `Variant analysis ${variantAnalysisLabel} no longer exists or is no longer accessible, stopping monitoring.`,
+          );
+          // Cancel monitoring on 404, as this probably means the user does not have access to it anymore
+          // e.g. lost access to repo, or repo was deleted
+          return;
+        }
+
         continue;
       }
 

extensions/ql-vscode/test/vscode-tests/activated-extension/variant-analysis/variant-analysis-monitor.test.ts

@@ -1,4 +1,5 @@
 import * as ghApiClient from "../../../../src/variant-analysis/gh-api/gh-api-client";
+import { RequestError } from "@octokit/request-error";
 import { VariantAnalysisMonitor } from "../../../../src/variant-analysis/variant-analysis-monitor";
 import {
   VariantAnalysis as VariantAnalysisApiResponse,
@@ -297,6 +298,55 @@ describe("Variant Analysis Monitor", () => {
         expect(mockEecuteCommand).not.toBeCalled();
       });
     });
+
+    describe("when a 404 is returned", () => {
+      let showAndLogWarningMessageSpy: jest.SpiedFunction<
+        typeof helpers.showAndLogWarningMessage
+      >;
+
+      beforeEach(async () => {
+        showAndLogWarningMessageSpy = jest
+          .spyOn(helpers, "showAndLogWarningMessage")
+          .mockResolvedValue(undefined);
+
+        const scannedRepos = createMockScannedRepos([
+          "pending",
+          "in_progress",
+          "in_progress",
+          "in_progress",
+          "pending",
+          "pending",
+        ]);
+        mockApiResponse = createMockApiResponse("in_progress", scannedRepos);
+        mockGetVariantAnalysis.mockResolvedValueOnce(mockApiResponse);
+
+        mockGetVariantAnalysis.mockRejectedValueOnce(
+          new RequestError("Not Found", 404, {
+            request: {
+              method: "GET",
+              url: "",
+              headers: {},
+            },
+            response: {
+              status: 404,
+              headers: {},
+              url: "",
+              data: {},
+            },
+          }),
+        );
+      });
+
+      it("should stop requesting the variant analysis", async () => {
+        await variantAnalysisMonitor.monitorVariantAnalysis(variantAnalysis);
+
+        expect(mockGetVariantAnalysis).toHaveBeenCalledTimes(2);
+        expect(showAndLogWarningMessageSpy).toHaveBeenCalledTimes(1);
+        expect(showAndLogWarningMessageSpy).toHaveBeenCalledWith(
+          expect.stringMatching(/not found/i),
+        );
+      });
+    });
   });
 
   function limitNumberOfAttemptsToMonitor() {