Merge pull request #1672 from github/robertbrignull/always_trigger_monitoring

When rehydrating, always trigger a monitoring command unless the variant analysis is fully complete

extensions/ql-vscode/.mocharc.json

@@ -1,6 +1,4 @@
 {
   "exit": true,
-  "require": [
+  "require": ["test/mocha.setup.js"]
-    "test/mocha.setup.js"
-  ]
 }

extensions/ql-vscode/src/query-history.ts

@@ -694,7 +694,7 @@ export class QueryHistoryManager extends DisposableObject {
         await this.remoteQueriesManager.rehydrateRemoteQuery(item.queryId, item.remoteQuery, item.status);
       }
       if (item.t === 'variant-analysis') {
-        await this.variantAnalysisManager.rehydrateVariantAnalysis(item.variantAnalysis, item.status);
+        await this.variantAnalysisManager.rehydrateVariantAnalysis(item.variantAnalysis);
       }
     }));
   }

extensions/ql-vscode/src/remote-queries/shared/variant-analysis.ts

@@ -130,6 +130,26 @@ export interface VariantAnalysisSubmission {
   }
 }
 
+export async function isVariantAnalysisComplete(
+  variantAnalysis: VariantAnalysis,
+  artifactDownloaded: (repo: VariantAnalysisScannedRepository) => Promise<boolean>
+): Promise<boolean> {
+  // It's only acceptable to have no scanned repos if the variant analysis is not in a final state.
+  // Otherwise it means the analysis hit some kind of internal error or there were no repos to scan.
+  if (variantAnalysis.scannedRepos === undefined || variantAnalysis.scannedRepos.length === 0) {
+    return variantAnalysis.status !== VariantAnalysisStatus.InProgress;
+  }
+
+  return (await Promise.all(variantAnalysis.scannedRepos.map(repo => isVariantAnalysisRepoComplete(repo, artifactDownloaded)))).every(x => x);
+}
+
+async function isVariantAnalysisRepoComplete(
+  repo: VariantAnalysisScannedRepository,
+  artifactDownloaded: (repo: VariantAnalysisScannedRepository) => Promise<boolean>
+): Promise<boolean> {
+  return hasRepoScanCompleted(repo) && (!repoHasDownloadableArtifact(repo) || await artifactDownloaded(repo));
+}
+
 /**
  * @param status
  * @returns whether the status is in a completed state, i.e. it cannot normally change state anymore
@@ -151,6 +171,14 @@ export function hasRepoScanCompleted(repo: VariantAnalysisScannedRepository): bo
   return isCompletedAnalysisRepoStatus(repo.analysisStatus);
 }
 
+/**
+ * @param repo
+ * @returns whether the repo scan has an artifact that can be downloaded
+ */
+export function repoHasDownloadableArtifact(repo: VariantAnalysisScannedRepository): boolean {
+  return repo.analysisStatus === VariantAnalysisRepoStatus.Succeeded;
+}
+
 /**
  * @param repos
  * @returns the total number of results. Will be `undefined` when there are no repos with results.

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

@@ -11,7 +11,9 @@ import {
   VariantAnalysisScannedRepository as ApiVariantAnalysisScannedRepository
 } from './gh-api/variant-analysis';
 import {
+  isVariantAnalysisComplete,
   VariantAnalysis, VariantAnalysisQueryLanguage,
+  VariantAnalysisScannedRepository,
   VariantAnalysisScannedRepositoryDownloadStatus,
   VariantAnalysisScannedRepositoryResult,
   VariantAnalysisScannedRepositoryState
@@ -24,7 +26,6 @@ import { getControllerRepo } from './run-remote-query';
 import { processUpdatedVariantAnalysis } from './variant-analysis-processor';
 import PQueue from 'p-queue';
 import { createTimestampFile, showAndLogErrorMessage } from '../helpers';
-import { QueryStatus } from '../query-status';
 import * as fs from 'fs-extra';
 
 export class VariantAnalysisManager extends DisposableObject implements VariantAnalysisViewManager<VariantAnalysisView> {
@@ -55,21 +56,24 @@ export class VariantAnalysisManager extends DisposableObject implements VariantA
     this.variantAnalysisResultsManager.onResultLoaded(this.onRepoResultLoaded.bind(this));
   }
 
-  public async rehydrateVariantAnalysis(variantAnalysis: VariantAnalysis, status: QueryStatus) {
+  public async rehydrateVariantAnalysis(variantAnalysis: VariantAnalysis) {
     if (!(await this.variantAnalysisRecordExists(variantAnalysis.id))) {
       // In this case, the variant analysis was deleted from disk, most likely because
       // it was purged by another workspace.
       this._onVariantAnalysisRemoved.fire(variantAnalysis);
     } else {
       await this.setVariantAnalysis(variantAnalysis);
-      if (status === QueryStatus.InProgress) {
+      if (!await isVariantAnalysisComplete(variantAnalysis, this.makeResultDownloadChecker(variantAnalysis))) {
-        // In this case, last time we checked, the query was still in progress.
-        // We need to setup the monitor to check for completion.
         await commands.executeCommand('codeQL.monitorVariantAnalysis', variantAnalysis);
       }
     }
   }
 
+  private makeResultDownloadChecker(variantAnalysis: VariantAnalysis): (repo: VariantAnalysisScannedRepository) => Promise<boolean> {
+    const storageLocation = this.getVariantAnalysisStorageLocation(variantAnalysis.id);
+    return (repo) => this.variantAnalysisResultsManager.isVariantAnalysisRepoDownloaded(storageLocation, repo.repository.fullName);
+  }
+
   public async removeVariantAnalysis(variantAnalysis: VariantAnalysis) {
     this.variantAnalysisResultsManager.removeAnalysisResults(variantAnalysis);
     await this.removeStorageDirectory(variantAnalysis.id);

extensions/ql-vscode/src/remote-queries/variant-analysis-results-manager.ts

@@ -146,7 +146,7 @@ export class VariantAnalysisResultsManager extends DisposableObject {
     throw new Error('Missing results file');
   }
 
-  private async isVariantAnalysisRepoDownloaded(
+  public async isVariantAnalysisRepoDownloaded(
     variantAnalysisStoragePath: string,
     repositoryFullName: string,
   ): Promise<boolean> {

extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-manager.test.ts

@@ -1,6 +1,6 @@
 import * as sinon from 'sinon';
 import { expect } from 'chai';
-import { CancellationTokenSource, extensions } from 'vscode';
+import { CancellationTokenSource, commands, extensions } from 'vscode';
 import { CodeQLExtensionInterface } from '../../../extension';
 import { logger } from '../../../logging';
 import * as config from '../../../config';
@@ -21,15 +21,17 @@ import { createMockVariantAnalysisRepoTask } from '../../factories/remote-querie
 import { CodeQLCliServer } from '../../../cli';
 import { storagePath } from '../global.helper';
 import { VariantAnalysisResultsManager } from '../../../remote-queries/variant-analysis-results-manager';
-import { VariantAnalysis } from '../../../remote-queries/shared/variant-analysis';
 import { createMockVariantAnalysis } from '../../factories/remote-queries/shared/variant-analysis';
+import { VariantAnalysis } from '../../../remote-queries/shared/variant-analysis';
+import * as VariantAnalysisModule from '../../../remote-queries/shared/variant-analysis';
+import { createTimestampFile } from '../../../helpers';
 
 describe('Variant Analysis Manager', async function() {
   let sandbox: sinon.SinonSandbox;
   let cli: CodeQLCliServer;
   let cancellationTokenSource: CancellationTokenSource;
   let variantAnalysisManager: VariantAnalysisManager;
-  let variantAnalysis: VariantAnalysisApiResponse;
+  let variantAnalysisApiResponse: VariantAnalysisApiResponse;
   let scannedRepos: ApiVariantAnalysisScannedRepository[];
   let getVariantAnalysisRepoStub: sinon.SinonStub;
   let getVariantAnalysisRepoResultStub: sinon.SinonStub;
@@ -45,7 +47,7 @@ describe('Variant Analysis Manager', async function() {
     cancellationTokenSource = new CancellationTokenSource();
 
     scannedRepos = createMockScannedRepos();
-    variantAnalysis = createMockApiResponse('in_progress', scannedRepos);
+    variantAnalysisApiResponse = createMockApiResponse('in_progress', scannedRepos);
 
     try {
       const extension = await extensions.getExtension<CodeQLExtensionInterface | Record<string, never>>('GitHub.vscode-codeql')!.activate();
@@ -68,7 +70,7 @@ describe('Variant Analysis Manager', async function() {
       try {
         await variantAnalysisManager.autoDownloadVariantAnalysisResult(
           scannedRepos[0],
-          variantAnalysis,
+          variantAnalysisApiResponse,
           cancellationTokenSource.token
         );
       } catch (error: any) {
@@ -105,7 +107,7 @@ describe('Variant Analysis Manager', async function() {
       it('should not try to download the result', async () => {
         await variantAnalysisManager.autoDownloadVariantAnalysisResult(
           scannedRepos[0],
-          variantAnalysis,
+          variantAnalysisApiResponse,
           cancellationTokenSource.token
         );
 
@@ -129,7 +131,7 @@ describe('Variant Analysis Manager', async function() {
 
           await variantAnalysisManager.autoDownloadVariantAnalysisResult(
             scannedRepos[0],
-            variantAnalysis,
+            variantAnalysisApiResponse,
             cancellationTokenSource.token
           );
 
@@ -139,7 +141,7 @@ describe('Variant Analysis Manager', async function() {
         it('should fetch a repo task', async () => {
           await variantAnalysisManager.autoDownloadVariantAnalysisResult(
             scannedRepos[0],
-            variantAnalysis,
+            variantAnalysisApiResponse,
             cancellationTokenSource.token
           );
 
@@ -149,7 +151,7 @@ describe('Variant Analysis Manager', async function() {
         it('should fetch a repo result', async () => {
           await variantAnalysisManager.autoDownloadVariantAnalysisResult(
             scannedRepos[0],
-            variantAnalysis,
+            variantAnalysisApiResponse,
             cancellationTokenSource.token
           );
 
@@ -161,9 +163,9 @@ describe('Variant Analysis Manager', async function() {
         it('should pop download tasks off the queue', async () => {
           const getResultsSpy = sandbox.spy(variantAnalysisManager, 'autoDownloadVariantAnalysisResult');
 
-          await variantAnalysisManager.enqueueDownload(scannedRepos[0], variantAnalysis, cancellationTokenSource.token);
+          await variantAnalysisManager.enqueueDownload(scannedRepos[0], variantAnalysisApiResponse, cancellationTokenSource.token);
-          await variantAnalysisManager.enqueueDownload(scannedRepos[1], variantAnalysis, cancellationTokenSource.token);
+          await variantAnalysisManager.enqueueDownload(scannedRepos[1], variantAnalysisApiResponse, cancellationTokenSource.token);
-          await variantAnalysisManager.enqueueDownload(scannedRepos[2], variantAnalysis, cancellationTokenSource.token);
+          await variantAnalysisManager.enqueueDownload(scannedRepos[2], variantAnalysisApiResponse, cancellationTokenSource.token);
 
           expect(variantAnalysisManager.downloadsQueueSize()).to.equal(0);
           expect(getResultsSpy).to.have.been.calledThrice;
@@ -194,4 +196,77 @@ describe('Variant Analysis Manager', async function() {
       });
     });
   });
+
+  describe('when rehydrating a query', async () => {
+    let variantAnalysis: VariantAnalysis;
+    let variantAnalysisRemovedSpy: sinon.SinonSpy;
+    let monitorVariantAnalysisCommandSpy: sinon.SinonSpy;
+
+    beforeEach(() => {
+      variantAnalysis = createMockVariantAnalysis();
+
+      variantAnalysisRemovedSpy = sinon.spy();
+      variantAnalysisManager.onVariantAnalysisRemoved(variantAnalysisRemovedSpy);
+
+      monitorVariantAnalysisCommandSpy = sinon.spy();
+      sandbox.stub(commands, 'executeCommand').callsFake(monitorVariantAnalysisCommandSpy);
+    });
+
+    describe('when variant analysis record doesn\'t exist', async () => {
+      it('should remove the variant analysis', async () => {
+        await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis);
+        sinon.assert.calledOnce(variantAnalysisRemovedSpy);
+      });
+
+      it('should not trigger a monitoring command', async () => {
+        await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis);
+        sinon.assert.notCalled(monitorVariantAnalysisCommandSpy);
+      });
+    });
+
+    describe('when variant analysis record does exist', async () => {
+      let variantAnalysisStorageLocation: string;
+
+      beforeEach(async () => {
+        variantAnalysisStorageLocation = variantAnalysisManager.getVariantAnalysisStorageLocation(variantAnalysis.id);
+        await createTimestampFile(variantAnalysisStorageLocation);
+      });
+
+      afterEach(() => {
+        fs.rmSync(variantAnalysisStorageLocation, { recursive: true });
+      });
+
+      describe('when the variant analysis is not complete', async () => {
+        beforeEach(() => {
+          sandbox.stub(VariantAnalysisModule, 'isVariantAnalysisComplete').resolves(false);
+        });
+
+        it('should not remove the variant analysis', async () => {
+          await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis);
+          sinon.assert.notCalled(variantAnalysisRemovedSpy);
+        });
+
+        it('should trigger a monitoring command', async () => {
+          await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis);
+          sinon.assert.calledWith(monitorVariantAnalysisCommandSpy, 'codeQL.monitorVariantAnalysis');
+        });
+      });
+
+      describe('when the variant analysis is complete', async () => {
+        beforeEach(() => {
+          sandbox.stub(VariantAnalysisModule, 'isVariantAnalysisComplete').resolves(true);
+        });
+
+        it('should not remove the variant analysis', async () => {
+          await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis);
+          sinon.assert.notCalled(variantAnalysisRemovedSpy);
+        });
+
+        it('should not trigger a monitoring command', async () => {
+          await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis);
+          sinon.assert.notCalled(monitorVariantAnalysisCommandSpy);
+        });
+      });
+    });
+  });
 });

extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts

@@ -13,7 +13,6 @@ import { CodeQLCliServer } from '../../../cli';
 import { storagePath } from '../global.helper';
 import { faker } from '@faker-js/faker';
 import * as ghApiClient from '../../../remote-queries/gh-api/gh-api-client';
-import { VariantAnalysisRepoTask } from '../../../remote-queries/gh-api/variant-analysis';
 
 describe(VariantAnalysisResultsManager.name, function() {
   this.timeout(10000);
@@ -47,15 +46,32 @@ describe(VariantAnalysisResultsManager.name, function() {
 
   describe('download', () => {
     let getOctokitStub: sinon.SinonStub;
-    let variantAnalysisStoragePath: string;
     const mockCredentials = {
       getOctokit: () => Promise.resolve({
         request: getOctokitStub
       })
     } as unknown as Credentials;
+    let dummyRepoTask = createMockVariantAnalysisRepoTask();
+    let variantAnalysisStoragePath: string;
+    let repoTaskStorageDirectory: string;
 
     beforeEach(async () => {
+      dummyRepoTask = createMockVariantAnalysisRepoTask();
+
       variantAnalysisStoragePath = path.join(storagePath, variantAnalysisId.toString());
+      repoTaskStorageDirectory = variantAnalysisResultsManager.getRepoStorageDirectory(variantAnalysisStoragePath, dummyRepoTask.repository.full_name);
+    });
+
+    afterEach(async () => {
+      if (fs.existsSync(variantAnalysisStoragePath)) {
+        fs.rmSync(variantAnalysisStoragePath, { recursive: true });
+      }
+    });
+
+    describe('isVariantAnalysisRepoDownloaded', () => {
+      it('should return false when no results are downloaded', async () => {
+        expect(await variantAnalysisResultsManager.isVariantAnalysisRepoDownloaded(variantAnalysisStoragePath, dummyRepoTask.repository.full_name)).to.equal(false);
+      });
     });
 
     describe('when the artifact_url is missing', async () => {
@@ -79,14 +95,9 @@ describe(VariantAnalysisResultsManager.name, function() {
     });
 
     describe('when the artifact_url is present', async () => {
-      let dummyRepoTask: VariantAnalysisRepoTask;
-      let storageDirectory: string;
       let arrayBuffer: ArrayBuffer;
 
       beforeEach(async () => {
-        dummyRepoTask = createMockVariantAnalysisRepoTask();
-
-        storageDirectory = variantAnalysisResultsManager.getRepoStorageDirectory(variantAnalysisStoragePath, dummyRepoTask.repository.full_name);
         const sourceFilePath = path.join(__dirname, '../../../../src/vscode-tests/cli-integration/data/variant-analysis-results.zip');
         arrayBuffer = fs.readFileSync(sourceFilePath).buffer;
 
@@ -96,11 +107,6 @@ describe(VariantAnalysisResultsManager.name, function() {
           .resolves(arrayBuffer);
       });
 
-      afterEach(async () => {
-        fs.removeSync(`${storageDirectory}/results.zip`);
-        fs.removeSync(`${storageDirectory}/results`);
-      });
-
       it('should call the API to download the results', async () => {
         await variantAnalysisResultsManager.download(
           mockCredentials,
@@ -120,7 +126,7 @@ describe(VariantAnalysisResultsManager.name, function() {
           variantAnalysisStoragePath
         );
 
-        expect(fs.existsSync(`${storageDirectory}/results.zip`)).to.be.true;
+        expect(fs.existsSync(`${repoTaskStorageDirectory}/results.zip`)).to.be.true;
       });
 
       it('should unzip the results in a `results/` folder', async () => {
@@ -131,7 +137,20 @@ describe(VariantAnalysisResultsManager.name, function() {
           variantAnalysisStoragePath
         );
 
-        expect(fs.existsSync(`${storageDirectory}/results/results.sarif`)).to.be.true;
+        expect(fs.existsSync(`${repoTaskStorageDirectory}/results/results.sarif`)).to.be.true;
+      });
+
+      describe('isVariantAnalysisRepoDownloaded', () => {
+        it('should return true once results are downloaded', async () => {
+          await variantAnalysisResultsManager.download(
+            mockCredentials,
+            variantAnalysisId,
+            dummyRepoTask,
+            variantAnalysisStoragePath
+          );
+
+          expect(await variantAnalysisResultsManager.isVariantAnalysisRepoDownloaded(variantAnalysisStoragePath, dummyRepoTask.repository.full_name)).to.equal(true);
+        });
       });
     });
   });

extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/variant-analysis-history.test.ts

@@ -121,9 +121,7 @@ describe('Variant Analyses and QueryHistoryManager', function() {
 
     expect(rehydrateVariantAnalysisStub).to.have.callCount(2);
     expect(rehydrateVariantAnalysisStub.getCall(0).args[0]).to.deep.eq(rawQueryHistory[0].variantAnalysis);
-    expect(rehydrateVariantAnalysisStub.getCall(0).args[1]).to.deep.eq(rawQueryHistory[0].status);
     expect(rehydrateVariantAnalysisStub.getCall(1).args[0]).to.deep.eq(rawQueryHistory[1].variantAnalysis);
-    expect(rehydrateVariantAnalysisStub.getCall(1).args[1]).to.deep.eq(rawQueryHistory[1].status);
 
     expect(qhm.treeDataProvider.allHistory[0]).to.deep.eq(rawQueryHistory[0]);
     expect(qhm.treeDataProvider.allHistory[1]).to.deep.eq(rawQueryHistory[1]);

extensions/ql-vscode/test/pure-tests/variant-analysis.test.ts

@@ -1,9 +1,6 @@
 import { expect } from 'chai';
-import {
+import { VariantAnalysis, parseVariantAnalysisQueryLanguage, VariantAnalysisQueryLanguage, VariantAnalysisStatus, isVariantAnalysisComplete, VariantAnalysisRepoStatus, getActionsWorkflowRunUrl } from '../../src/remote-queries/shared/variant-analysis';
-  getActionsWorkflowRunUrl,
+import { createMockScannedRepo } from '../../src/vscode-tests/factories/remote-queries/shared/scanned-repositories';
-  parseVariantAnalysisQueryLanguage,
-  VariantAnalysisQueryLanguage
-} from '../../src/remote-queries/shared/variant-analysis';
 import { createMockVariantAnalysis } from '../../src/vscode-tests/factories/remote-queries/shared/variant-analysis';
 
 describe('parseVariantAnalysisQueryLanguage', () => {
@@ -16,6 +13,95 @@ describe('parseVariantAnalysisQueryLanguage', () => {
   });
 });
 
+describe('isVariantAnalysisComplete', async () => {
+  let variantAnalysis: VariantAnalysis;
+  const uncallableArtifactDownloadChecker = () => { throw new Error('Should not be called'); };
+
+  beforeEach(() => {
+    variantAnalysis = createMockVariantAnalysis();
+  });
+
+  describe('when variant analysis status is InProgress', async () => {
+    beforeEach(() => {
+      variantAnalysis.status = VariantAnalysisStatus.InProgress;
+    });
+
+    describe('when scanned repos is undefined', async () => {
+      it('should say the variant analysis is not complete', async () => {
+        variantAnalysis.scannedRepos = undefined;
+        expect(await isVariantAnalysisComplete(variantAnalysis, uncallableArtifactDownloadChecker)).to.equal(false);
+      });
+    });
+
+    describe('when scanned repos is non-empty', async () => {
+      describe('when not all results are downloaded', async () => {
+        it('should say the variant analysis is not complete', async () => {
+          expect(await isVariantAnalysisComplete(variantAnalysis, async () => false)).to.equal(false);
+        });
+      });
+
+      describe('when all results are downloaded', async () => {
+        it('should say the variant analysis is complete', async () => {
+          expect(await isVariantAnalysisComplete(variantAnalysis, async () => true)).to.equal(false);
+        });
+      });
+    });
+  });
+
+  for (const variantAnalysisStatus of [
+    VariantAnalysisStatus.Succeeded,
+    VariantAnalysisStatus.Failed,
+    VariantAnalysisStatus.Canceled
+  ]) {
+    describe(`when variant analysis status is ${variantAnalysisStatus}`, async () => {
+      beforeEach(() => {
+        variantAnalysis.status = variantAnalysisStatus;
+      });
+
+      describe('when scanned repos is undefined', async () => {
+        it('should say the variant analysis is complete', async () => {
+          variantAnalysis.scannedRepos = undefined;
+          expect(await isVariantAnalysisComplete(variantAnalysis, uncallableArtifactDownloadChecker)).to.equal(true);
+        });
+      });
+
+      describe('when scanned repos is empty', async () => {
+        it('should say the variant analysis is complete', async () => {
+          variantAnalysis.scannedRepos = [];
+          expect(await isVariantAnalysisComplete(variantAnalysis, uncallableArtifactDownloadChecker)).to.equal(true);
+        });
+      });
+
+      describe('when a repo scan is still in progress', async () => {
+        it('should say the variant analysis is not complete', async () => {
+          variantAnalysis.scannedRepos = [
+            createMockScannedRepo('in-progress-repo', false, VariantAnalysisRepoStatus.InProgress),
+          ];
+          expect(await isVariantAnalysisComplete(variantAnalysis, async () => false)).to.equal(false);
+        });
+      });
+
+      describe('when not all results are downloaded', async () => {
+        it('should say the variant analysis is not complete', async () => {
+          variantAnalysis.scannedRepos = [
+            createMockScannedRepo('in-progress-repo', false, VariantAnalysisRepoStatus.Succeeded),
+          ];
+          expect(await isVariantAnalysisComplete(variantAnalysis, async () => false)).to.equal(false);
+        });
+      });
+
+      describe('when all results are downloaded', async () => {
+        it('should say the variant analysis is complete', async () => {
+          variantAnalysis.scannedRepos = [
+            createMockScannedRepo('in-progress-repo', false, VariantAnalysisRepoStatus.Succeeded),
+          ];
+          expect(await isVariantAnalysisComplete(variantAnalysis, async () => true)).to.equal(true);
+        });
+      });
+    });
+  }
+});
+
 describe('getActionsWorkflowRunUrl', () => {
   it('should get the run url', () => {
     const variantAnalysis = createMockVariantAnalysis();