When rehydrating, always trigger a monitoring command if variant analysis is not complete

Author: robertbrignull

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

@@ -151,6 +151,14 @@ export function hasRepoScanCompleted(repo: VariantAnalysisScannedRepository): bo
   return isCompletedAnalysisRepoStatus(repo.analysisStatus);
 }
 
+/**
+ * @param repo
+ * @returns whether the repo scan is complete and has results that can be downloaded
+ */
+export function repoScanHasResults(repo: VariantAnalysisScannedRepository): boolean {
+  return repo.analysisStatus === VariantAnalysisRepoStatus.Succeeded && (repo.resultCount || 0) > 0;
+}
+
 /**
  * @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,10 +11,14 @@ import {
   VariantAnalysisScannedRepository as ApiVariantAnalysisScannedRepository
 } from './gh-api/variant-analysis';
 import {
+  hasRepoScanCompleted,
+  repoScanHasResults,
   VariantAnalysis, VariantAnalysisQueryLanguage,
+  VariantAnalysisScannedRepository,
   VariantAnalysisScannedRepositoryDownloadStatus,
   VariantAnalysisScannedRepositoryResult,
-  VariantAnalysisScannedRepositoryState
+  VariantAnalysisScannedRepositoryState,
+  VariantAnalysisStatus
 } from './shared/variant-analysis';
 import { getErrorMessage } from '../pure/helpers-pure';
 import { VariantAnalysisView } from './variant-analysis-view';
@@ -24,7 +28,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';
 
 
@@ -56,22 +59,42 @@ 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 {
       this.variantAnalyses.set(variantAnalysis.id, variantAnalysis);
       await this.getView(variantAnalysis.id)?.updateView(variantAnalysis);
-      if (status === QueryStatus.InProgress) {
-        // In this case, last time we checked, the query was still in progress.
-        // We need to setup the monitor to check for completion.
+
+      if (!await this.isVariantAnalysisComplete(variantAnalysis)) {
         await commands.executeCommand('codeQL.monitorVariantAnalysis', variantAnalysis);
       }
     }
   }
 
+  public async isVariantAnalysisComplete(variantAnalysis: VariantAnalysis): 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 => this.isVariantAnalysisRepoComplete(variantAnalysis.id, repo)))).every(x => x);
+  }
+
+  private async isVariantAnalysisRepoComplete(variantAnalysisId: number, repo: VariantAnalysisScannedRepository): Promise<boolean> {
+    if (!hasRepoScanCompleted(repo)) {
+      return false;
+    } else if (!repoScanHasResults(repo)) {
+      return true;
+    } else {
+      const storageLocation = this.getVariantAnalysisStorageLocation(variantAnalysisId);
+      return await this.variantAnalysisResultsManager.isVariantAnalysisRepoDownloaded(storageLocation, repo.repository.fullName);
+    }
+  }
+
   public async removeVariantAnalysis(variantAnalysis: VariantAnalysis) {
     this.variantAnalysisResultsManager.removeAnalysisResults(variantAnalysis);
     await this.removeStorageDirectory(variantAnalysis.id);
@@ -234,7 +257,7 @@ export class VariantAnalysisManager extends DisposableObject implements VariantA
    * used by the query history manager to determine when the directory
    * should be deleted.
    */
-  private async prepareStorageDirectory(variantAnalysisId: number): Promise<void> {
+  public async prepareStorageDirectory(variantAnalysisId: number): Promise<void> {
     await createTimestampFile(this.getVariantAnalysisStorageLocation(variantAnalysisId));
   }
 

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, Disposable, extensions } from 'vscode';
 import { CodeQLExtensionInterface } from '../../../extension';
 import { logger } from '../../../logging';
 import * as config from '../../../config';
@@ -21,6 +21,8 @@ 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 { createMockVariantAnalysis } from '../../factories/remote-queries/shared/variant-analysis';
+import { VariantAnalysis, VariantAnalysisStatus } from '../../../remote-queries/shared/variant-analysis';
 
 describe('Variant Analysis Manager', async function() {
   let sandbox: sinon.SinonSandbox;
@@ -165,4 +167,168 @@ describe('Variant Analysis Manager', async function() {
       });
     });
   });
+
+  describe('when rehydrating a query', async () => {
+    let variantAnalysis: VariantAnalysis;
+    let variantAnalysisRemovedFired = false;
+    let onVariantAnalysisRemovedListener: Disposable;
+    let monitorVariantAnalysisCommandCalled = false;
+
+    beforeEach(() => {
+      variantAnalysis = createMockVariantAnalysis();
+
+      variantAnalysisRemovedFired = false;
+      onVariantAnalysisRemovedListener = variantAnalysisManager.onVariantAnalysisRemoved(() => {
+        variantAnalysisRemovedFired = true;
+      });
+
+      monitorVariantAnalysisCommandCalled = false;
+      sandbox.stub(commands, 'executeCommand').callsFake((command: string) => {
+        if (command === 'codeQL.monitorVariantAnalysis') {
+          monitorVariantAnalysisCommandCalled = true;
+        }
+        return Promise.resolve();
+      });
+    });
+
+    afterEach(() => {
+      onVariantAnalysisRemovedListener.dispose();
+    });
+
+    describe('when variant analysis record doesn\'t exist', async () => {
+      it('should remove the variant analysis', async () => {
+        await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis);
+        expect(variantAnalysisRemovedFired).to.equal(true);
+      });
+
+      it('should not trigger a monitoring command', async () => {
+        await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis);
+        expect(monitorVariantAnalysisCommandCalled).to.equal(false);
+      });
+    });
+
+    describe('when variant analysis record does exist', async () => {
+      let variantAnalysisStorageLocation: string;
+
+      beforeEach(async () => {
+        variantAnalysisStorageLocation = variantAnalysisManager.getVariantAnalysisStorageLocation(variantAnalysis.id);
+        await variantAnalysisManager.prepareStorageDirectory(variantAnalysis.id);
+      });
+
+      afterEach(() => {
+        fs.rmSync(variantAnalysisStorageLocation, { recursive: true });
+      });
+
+      describe('when the variant analysis is not complete', async () => {
+        beforeEach(() => {
+          sinon.stub(variantAnalysisManager, 'isVariantAnalysisComplete').resolves(false);
+        });
+
+        it('should not remove the variant analysis', async () => {
+          await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis);
+          expect(variantAnalysisRemovedFired).to.equal(false);
+        });
+
+        it('should trigger a monitoring command', async () => {
+          await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis);
+          expect(monitorVariantAnalysisCommandCalled).to.equal(true);
+        });
+      });
+
+      describe('when the variant analysis is complete', async () => {
+        beforeEach(() => {
+          sinon.stub(variantAnalysisManager, 'isVariantAnalysisComplete').resolves(true);
+        });
+
+        it('should not remove the variant analysis', async () => {
+          await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis);
+          expect(variantAnalysisRemovedFired).to.equal(false);
+        });
+
+        it('should not trigger a monitoring command', async () => {
+          await variantAnalysisManager.rehydrateVariantAnalysis(variantAnalysis);
+          expect(monitorVariantAnalysisCommandCalled).to.equal(false);
+        });
+      });
+    });
+  });
+
+  describe('isVariantAnalysisComplete', async () => {
+    let variantAnalysis: VariantAnalysis;
+
+    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(variantAnalysisManager.isVariantAnalysisComplete(variantAnalysis)).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 () => {
+            sinon.stub(variantAnalysisResultsManager, 'isVariantAnalysisRepoDownloaded').resolves(false);
+            expect(variantAnalysisManager.isVariantAnalysisComplete(variantAnalysis)).to.equal(false);
+          });
+        });
+
+        describe('when all results are downloaded', async () => {
+          it('should say the variant analysis is complete', async () => {
+            sinon.stub(variantAnalysisResultsManager, 'isVariantAnalysisRepoDownloaded').resolves(true);
+            expect(variantAnalysisManager.isVariantAnalysisComplete(variantAnalysis)).to.equal(true);
+          });
+        });
+      });
+    });
+
+    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(variantAnalysisManager.isVariantAnalysisComplete(variantAnalysis)).to.equal(true);
+          });
+        });
+
+        describe('when scanned repos is empty', async () => {
+          it('should say the variant analysis is complete', async () => {
+            variantAnalysis.scannedRepos = [];
+            expect(variantAnalysisManager.isVariantAnalysisComplete(variantAnalysis)).to.equal(true);
+          });
+        });
+
+        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 () => {
+              sinon.stub(variantAnalysisResultsManager, 'isVariantAnalysisRepoDownloaded').resolves(false);
+              expect(variantAnalysisManager.isVariantAnalysisComplete(variantAnalysis)).to.equal(false);
+            });
+          });
+
+          describe('when all results are downloaded', async () => {
+            it('should say the variant analysis is complete', async () => {
+              sinon.stub(variantAnalysisResultsManager, 'isVariantAnalysisRepoDownloaded').resolves(true);
+              expect(variantAnalysisManager.isVariantAnalysisComplete(variantAnalysis)).to.equal(true);
+            });
+          });
+        });
+      });
+    }
+  });
 });