Clean up the database if it will be uploaded #3010
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors database cleanup to ensure databases are properly cleaned before upload operations. The primary purpose is to address the issue where intermediate results may still be written to databases even with --expect-discarded-cache when there is high RAM pressure, requiring cleanup before uploading.
Key changes include:
- Moving database cleanup logic from a separate stage into the respective upload functions
- Removing the standalone
runCleanupfunction andcleanupLevelparameter fromrunQueries - Adding
databaseCleanupClustermethod to the CodeQL interface for cleaning all databases in a cluster
Reviewed Changes
Copilot reviewed 14 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/codeql.ts | Adds new databaseCleanupCluster method to CodeQL interface and implementation |
| src/database-upload.ts | Integrates database cleanup directly into upload flow with "clear" level cleanup |
| src/overlay-database-utils.ts | Adds database cleanup at "overlay" level before caching overlay databases |
| src/analyze.ts | Removes standalone cleanup function and cleanupLevel parameter from runQueries |
| src/analyze-action.ts | Updates workflow to handle cleanup-level input deprecation and reorders upload operations |
| src/database-upload.test.ts | Updates tests to accommodate new CodeQL parameter and mock cleanup method |
| src/analyze.test.ts | Removes cleanupLevel parameter from test calls |
Comments suppressed due to low confidence (1)
src/database-upload.ts:19
- Adding the
codeqlparameter to this function is a breaking change to the API. Consider documenting this breaking change in the PR description or changelog to help consumers of this function.
repositoryNwo: RepositoryNwo,
codeql: CodeQL,
config: Config,
apiDetails: GitHubApiDetails,
logger: Logger,
): Promise<void> {
| "The 'cleanup-level' input is ignored since the CodeQL Action no longer writes intermediate results to the database. This input can safely be removed from your workflow.", | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
The cleanup-level deprecation warning has been moved earlier in the function but the logic is duplicated - this same warning appears in the original location. Consider removing this duplication to avoid confusing users with multiple identical warnings.
Copilot uses AI. Check for mistakes.
src/codeql.ts
Outdated
| @@ -155,6 +155,10 @@ export interface CodeQL { | |||
| * Run 'codeql database cleanup'. | |||
| */ | |||
| databaseCleanup(databasePath: string, cleanupLevel: string): Promise<void>; | |||
There was a problem hiding this comment.
Shall we remove the databaseCleanup() function, since it is now replaced by databaseCleanupCluster() and no longer used?
Even with
--expect-discarded-cache, intermediate results may still be written to the database if there is high RAM pressure. So clean up the database if we will upload it.There are a couple of approaches to doing this. I went with moving the cleanup into the upload stage to avoid needing to pull out separate logic about whether the upload should take place or not (and potentially needing to run this logic twice). This does make the cleanup slightly less transparent — I added comments and don't think this is too problematic but feedback welcome.
Merge / deployment checklist