Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion extensions/ql-vscode/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ import { ExtensionApp } from "./common/vscode/vscode-app";
import { RepositoriesFilterSortStateWithIds } from "./pure/variant-analysis-filter-sort";
import { DbModule } from "./databases/db-module";
import { redactableError } from "./pure/errors";
import { QueryHistoryDirs } from "./query-history/query-history-dirs";

/**
* extension.ts
Expand Down Expand Up @@ -649,13 +650,18 @@ async function activateWithInstalledDistribution(
);

void extLogger.log("Initializing query history.");
const queryHistoryDirs: QueryHistoryDirs = {
localQueriesDirPath: queryStorageDir,
variantAnalysesDirPath: variantAnalysisStorageDir,
};

const qhm = new QueryHistoryManager(
qs,
dbm,
localQueryResultsView,
variantAnalysisManager,
evalLogViewer,
queryStorageDir,
queryHistoryDirs,
ctx,
queryHistoryConfigurationListener,
labelProvider,
Expand Down
5 changes: 5 additions & 0 deletions extensions/ql-vscode/src/pure/files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,3 +67,8 @@ export function pathsEqual(
}
return path1 === path2;
}

export async function readDirFullPaths(path: string): Promise<string[]> {
const baseNames = await readdir(path);
return baseNames.map((baseName) => join(path, baseName));
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export interface QueryHistoryDirs {
localQueriesDirPath: string;
variantAnalysesDirPath: string;
}
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ import { VariantAnalysisHistoryItem } from "./variant-analysis-history-item";
import { getTotalResultCount } from "../variant-analysis/shared/variant-analysis";
import { HistoryTreeDataProvider } from "./history-tree-data-provider";
import { redactableError } from "../pure/errors";
import { QueryHistoryDirs } from "./query-history-dirs";

/**
* query-history-manager.ts
Expand Down Expand Up @@ -139,7 +140,7 @@ export class QueryHistoryManager extends DisposableObject {
private readonly localQueriesResultsView: ResultsView,
private readonly variantAnalysisManager: VariantAnalysisManager,
private readonly evalLogViewer: EvalLogViewer,
private readonly queryStorageDir: string,
private readonly queryHistoryDirs: QueryHistoryDirs,
ctx: ExtensionContext,
private readonly queryHistoryConfigListener: QueryHistoryConfig,
private readonly labelProvider: HistoryItemLabelProvider,
Expand Down Expand Up @@ -389,7 +390,7 @@ export class QueryHistoryManager extends DisposableObject {
ONE_HOUR_IN_MS,
TWO_HOURS_IN_MS,
queryHistoryConfigListener.ttlInMillis,
this.queryStorageDir,
this.queryHistoryDirs,
qhm,
ctx,
),
Expand Down
39 changes: 28 additions & 11 deletions extensions/ql-vscode/src/query-history/query-history-scrubber.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { pathExists, readdir, stat, remove, readFile } from "fs-extra";
import { pathExists, stat, remove, readFile } from "fs-extra";
import { EOL } from "os";
import { join } from "path";
import { Disposable, ExtensionContext } from "vscode";
import { extLogger } from "../common";
import { readDirFullPaths } from "../pure/files";
import { QueryHistoryDirs } from "./query-history-dirs";
import { QueryHistoryManager } from "./query-history-manager";

const LAST_SCRUB_TIME_KEY = "lastScrubTime";
Expand All @@ -23,14 +25,14 @@ type Counter = {
* @param wakeInterval How often to check to see if the job should run.
* @param throttleTime How often to actually run the job.
* @param maxQueryTime The maximum age of a query before is ready for deletion.
* @param queryDirectory The directory containing all queries.
* @param queryHistoryDirs The directories containing all query history information.
* @param ctx The extension context.
*/
export function registerQueryHistoryScrubber(
wakeInterval: number,
throttleTime: number,
maxQueryTime: number,
queryDirectory: string,
queryHistoryDirs: QueryHistoryDirs,
qhm: QueryHistoryManager,
ctx: ExtensionContext,

Expand All @@ -42,7 +44,7 @@ export function registerQueryHistoryScrubber(
wakeInterval,
throttleTime,
maxQueryTime,
queryDirectory,
queryHistoryDirs,
qhm,
ctx,
counter,
Expand All @@ -58,7 +60,7 @@ export function registerQueryHistoryScrubber(
async function scrubQueries(
throttleTime: number,
maxQueryTime: number,
queryDirectory: string,
queryHistoryDirs: QueryHistoryDirs,
qhm: QueryHistoryManager,
ctx: ExtensionContext,
counter?: Counter,
Expand All @@ -74,18 +76,33 @@ async function scrubQueries(
let scrubCount = 0; // total number of directories deleted
try {
counter?.increment();
void extLogger.log("Scrubbing query directory. Removing old queries.");
if (!(await pathExists(queryDirectory))) {
void extLogger.log(
"Cleaning up query history directories. Removing old entries.",
);

if (!(await pathExists(queryHistoryDirs.localQueriesDirPath))) {
void extLogger.log(
`Cannot clean up query history directories. Local queries directory does not exist: ${queryHistoryDirs.localQueriesDirPath}`,
);
return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to return here? If the local query history directory doesn't exist we can still clean up the variant analysis directory. I suppose we do expect both dirs to exist, so this is still an unexpected error, but do we want to abort fully or try to continue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that since this is an exceptional case then it's okay to abort. I don't have a strong opinion though - happy to change it to be more resilient.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right it should be an exceptional case that doesn't happen in practice. Also it's not like one of local or remote queries is a more central part of the extension, so it's not like a user would want to only use one and therefore want to run without one of the directories existing. And we recreate the directories on extension startup. So if this does happen it hopefully won't be a long term problem and will be fixed by restarting.

That's a lot of words, but in short I think it's fine as it is. Feel free to abort if any of the directories don't exist and keep the code simple.

}
if (!(await pathExists(queryHistoryDirs.variantAnalysesDirPath))) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok to do since https://github.com/github/vscode-codeql/blob/main/extensions/ql-vscode/src/extension.ts#L628 is not checking for canary flags.

void extLogger.log(
`Cannot scrub. Query directory does not exist: ${queryDirectory}`,
`Cannot clean up query history directories. Variant analyses directory does not exist: ${queryHistoryDirs.variantAnalysesDirPath}`,
);
return;
}

const baseNames = await readdir(queryDirectory);
const localQueryDirPaths = await readDirFullPaths(
queryHistoryDirs.localQueriesDirPath,
);
const variantAnalysisDirPaths = await readDirFullPaths(
queryHistoryDirs.variantAnalysesDirPath,
);
const allDirPaths = [...localQueryDirPaths, ...variantAnalysisDirPaths];

const errors: string[] = [];
for (const baseName of baseNames) {
const dir = join(queryDirectory, baseName);
for (const dir of allDirPaths) {
const scrubResult = await scrubDirectory(dir, now, maxQueryTime);
if (scrubResult.errorMsg) {
errors.push(scrubResult.errorMsg);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { QueryHistoryDirs } from "../../../src/query-history/query-history-dirs";

export function createMockQueryHistoryDirs({
localQueriesDirPath = "mock-local-queries-dir-path",
variantAnalysesDirPath = "mock-variant-analyses-dir-path",
}: {
localQueriesDirPath?: string;
variantAnalysesDirPath?: string;
} = {}): QueryHistoryDirs {
return {
localQueriesDirPath,
variantAnalysesDirPath,
};
}
15 changes: 15 additions & 0 deletions extensions/ql-vscode/test/unit-tests/pure/files.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
gatherQlFiles,
getDirectoryNamesInsidePath,
pathsEqual,
readDirFullPaths,
} from "../../../src/pure/files";

describe("files", () => {
Expand Down Expand Up @@ -100,6 +101,20 @@ describe("files", () => {
expect(result).toEqual(["sub-folder"]);
});
});

describe("readDirFullPaths", () => {
it("should return all files with full path", async () => {
const file1 = join(dataDir, "compute-default-strings.ql");
const file2 = join(dataDir, "multiple-result-sets.ql");
const file3 = join(dataDir, "query.ql");

const paths = await readDirFullPaths(dataDir);

expect(paths.some((path) => path === file1)).toBe(true);
expect(paths.some((path) => path === file2)).toBe(true);
expect(paths.some((path) => path === file3)).toBe(true);
});
});
});

describe("pathsEqual", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
SortOrder,
} from "../../../../src/query-history/history-tree-data-provider";
import { QueryHistoryManager } from "../../../../src/query-history/query-history-manager";
import { createMockQueryHistoryDirs } from "../../../factories/query-history/query-history-dirs";

describe("HistoryTreeDataProvider", () => {
const mockExtensionLocation = join(tmpDir.name, "mock-extension-location");
Expand Down Expand Up @@ -425,7 +426,7 @@ describe("HistoryTreeDataProvider", () => {
localQueriesResultsViewStub,
variantAnalysisManagerStub,
{} as EvalLogViewer,
"xxx",
createMockQueryHistoryDirs(),
{
globalStorageUri: vscode.Uri.file(mockExtensionLocation),
extensionPath: vscode.Uri.file("/x/y/z").fsPath,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { QuickPickItem, TextEditor } from "vscode";
import { WebviewReveal } from "../../../../src/interface-utils";
import * as helpers from "../../../../src/helpers";
import { mockedObject } from "../../utils/mocking.helpers";
import { createMockQueryHistoryDirs } from "../../../factories/query-history/query-history-dirs";

describe("QueryHistoryManager", () => {
const mockExtensionLocation = join(tmpDir.name, "mock-extension-location");
Expand Down Expand Up @@ -1150,7 +1151,7 @@ describe("QueryHistoryManager", () => {
localQueriesResultsViewStub,
variantAnalysisManagerStub,
{} as EvalLogViewer,
"xxx",
createMockQueryHistoryDirs(),
{
globalStorageUri: vscode.Uri.file(mockExtensionLocation),
extensionPath: vscode.Uri.file("/x/y/z").fsPath,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ describe("query history scrubber", () => {
ONE_HOUR_IN_MS,
TWO_HOURS_IN_MS,
LESS_THAN_ONE_DAY,
dir,
{ localQueriesDirPath: dir, variantAnalysesDirPath: dir },
mockedObject<QueryHistoryManager>({
removeDeletedQueries: () => {
return Promise.resolve();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { QueryRunner } from "../../../../src/queryRunner";
import { VariantAnalysisManager } from "../../../../src/variant-analysis/variant-analysis-manager";
import { QueryHistoryManager } from "../../../../src/query-history/query-history-manager";
import { mockedObject } from "../../utils/mocking.helpers";
import { createMockQueryHistoryDirs } from "../../../factories/query-history/query-history-dirs";

// set a higher timeout since recursive delete may take a while, expecially on Windows.
jest.setTimeout(120000);
Expand Down Expand Up @@ -74,7 +75,7 @@ describe("Variant Analyses and QueryHistoryManager", () => {
localQueriesResultsViewStub,
variantAnalysisManagerStub,
{} as EvalLogViewer,
STORAGE_DIR,
createMockQueryHistoryDirs({ localQueriesDirPath: STORAGE_DIR }),
mockedObject<ExtensionContext>({
globalStorageUri: Uri.file(STORAGE_DIR),
storageUri: undefined,
Expand Down