From ff3b6091c6f5a495e3dc8ff99cabe285731a8457 Mon Sep 17 00:00:00 2001
From: Koen Vlaswinkel <koesie10@github.com>
Date: Tue, 29 Nov 2022 11:12:46 +0100
Subject: [PATCH] Stop variant analysis monitor after removing it

This will stop the variant analysis monitor from monitoring when a
variant analysis is removed from the query history. Since the variant
analysis monitor cannot depend on the variant analysis manager (this
would create a circular dependency), a function is passed into the
variant analysis monitor for checking whether the variant analysis
should be cancelled.

This commit will also ensure that even if a variant analysis comes in
through the `onVariantAnalysisChange` callback, it won't be added to
the variant analysis map of the manager.
---
 .../remote-queries/variant-analysis-manager.ts | 17 ++++++++++++++++-
 .../remote-queries/variant-analysis-monitor.ts | 11 ++++++++++-
 .../variant-analysis-monitor.test.ts           | 18 +++++++++++++++++-
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts b/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts
index acab75690..532f5bf7b 100644
--- a/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts
+++ b/extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts
@@ -94,7 +94,12 @@ export class VariantAnalysisManager
     private readonly variantAnalysisResultsManager: VariantAnalysisResultsManager,
   ) {
     super();
-    this.variantAnalysisMonitor = this.push(new VariantAnalysisMonitor(ctx));
+    this.variantAnalysisMonitor = this.push(
+      new VariantAnalysisMonitor(
+        ctx,
+        this.shouldCancelMonitorVariantAnalysis.bind(this),
+      ),
+    );
     this.variantAnalysisMonitor.onVariantAnalysisChange(
       this.onVariantAnalysisUpdated.bind(this),
     );
@@ -319,6 +324,12 @@ export class VariantAnalysisManager
     return await fs.pathExists(filePath);
   }
 
+  private async shouldCancelMonitorVariantAnalysis(
+    variantAnalysisId: number,
+  ): Promise<boolean> {
+    return !this.variantAnalyses.has(variantAnalysisId);
+  }
+
   public async onVariantAnalysisUpdated(
     variantAnalysis: VariantAnalysis | undefined,
   ): Promise<void> {
@@ -326,6 +337,10 @@ export class VariantAnalysisManager
       return;
     }
 
+    if (!this.variantAnalyses.has(variantAnalysis.id)) {
+      return;
+    }
+
     await this.setVariantAnalysis(variantAnalysis);
     this._onVariantAnalysisStatusUpdated.fire(variantAnalysis);
   }
diff --git a/extensions/ql-vscode/src/remote-queries/variant-analysis-monitor.ts b/extensions/ql-vscode/src/remote-queries/variant-analysis-monitor.ts
index 8f05cd8a7..875e857a9 100644
--- a/extensions/ql-vscode/src/remote-queries/variant-analysis-monitor.ts
+++ b/extensions/ql-vscode/src/remote-queries/variant-analysis-monitor.ts
@@ -29,7 +29,12 @@ export class VariantAnalysisMonitor extends DisposableObject {
   );
   readonly onVariantAnalysisChange = this._onVariantAnalysisChange.event;
 
-  constructor(private readonly extensionContext: ExtensionContext) {
+  constructor(
+    private readonly extensionContext: ExtensionContext,
+    private readonly shouldCancelMonitor: (
+      variantAnalysisId: number,
+    ) => Promise<boolean>,
+  ) {
     super();
   }
 
@@ -52,6 +57,10 @@ export class VariantAnalysisMonitor extends DisposableObject {
         return { status: "Canceled" };
       }
 
+      if (await this.shouldCancelMonitor(variantAnalysis.id)) {
+        return { status: "Canceled" };
+      }
+
       const variantAnalysisSummary = await ghApiClient.getVariantAnalysis(
         credentials,
         variantAnalysis.controllerRepo.id,
diff --git a/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-monitor.test.ts b/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-monitor.test.ts
index 6b4db96aa..04102b6aa 100644
--- a/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-monitor.test.ts
+++ b/extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-monitor.test.ts
@@ -37,6 +37,7 @@ describe("Variant Analysis Monitor", async function () {
   let mockGetVariantAnalysis: sinon.SinonStub;
   let cancellationTokenSource: CancellationTokenSource;
   let variantAnalysisMonitor: VariantAnalysisMonitor;
+  let shouldCancelMonitor: sinon.SinonStub;
   let variantAnalysis: VariantAnalysis;
   let variantAnalysisManager: VariantAnalysisManager;
   let mockGetDownloadResult: sinon.SinonStub;
@@ -44,6 +45,7 @@ describe("Variant Analysis Monitor", async function () {
   beforeEach(async () => {
     sandbox = sinon.createSandbox();
     sandbox.stub(config, "isVariantAnalysisLiveResultsEnabled").returns(false);
+    shouldCancelMonitor = sinon.stub();
 
     cancellationTokenSource = new CancellationTokenSource();
 
@@ -55,7 +57,10 @@ describe("Variant Analysis Monitor", async function () {
           "GitHub.vscode-codeql",
         )!
         .activate();
-      variantAnalysisMonitor = new VariantAnalysisMonitor(extension.ctx);
+      variantAnalysisMonitor = new VariantAnalysisMonitor(
+        extension.ctx,
+        shouldCancelMonitor,
+      );
     } catch (e) {
       fail(e as Error);
     }
@@ -112,6 +117,17 @@ describe("Variant Analysis Monitor", async function () {
       expect(result).to.eql({ status: "Canceled" });
     });
 
+    it("should return early if variant analysis should be cancelled", async () => {
+      shouldCancelMonitor.resolves(true);
+
+      const result = await variantAnalysisMonitor.monitorVariantAnalysis(
+        variantAnalysis,
+        cancellationTokenSource.token,
+      );
+
+      expect(result).to.eql({ status: "Canceled" });
+    });
+
     describe("when the variant analysis fails", async () => {
       let mockFailedApiResponse: VariantAnalysisApiResponse;