Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up old distributions #3763

Merged
merged 6 commits into from
Oct 22, 2024
Merged

Clean up old distributions #3763

merged 6 commits into from
Oct 22, 2024

Conversation

koesie10
Copy link
Member

This adds a background process for cleaning up old distributions (i.e. old extension-managed CodeQL versions). These could be left behind as a result of a bug (which is fixed by #3762).

On start-up (after extension activation), it will asynchronously list all directories in the globalStorage directory. It will then select all directories which match the pattern distribution<n> where <n> is a number. Then, it will sequentially remove all folders which have a lower folder index (i.e. the <n>) than the currently installed CodeQL CLI. It will wait 10 seconds between each directory to avoid overloading the system. The remove call itself should also be asynchronous, so it should not block the extension thread.

I've tested this locally by copying my current distribution a couple of times to some other distribution folders and it does remove the correct directories. I've also added some tests for this.

@koesie10 koesie10 requested a review from a team as a code owner October 16, 2024 13:47
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

This looks generally good. My only question is how does this behave if we have multiple workspaces open each trying to delete the same set of distributions? I'm guessing one of them will throw an error, but continue deleting.

Could you make sure that an error message is not sent to the log if a distribution has already been deleted when the cleaner tries to clean it?

@aeisenberg
Copy link
Contributor

We also have a similar process to remove orphaned databases.

@koesie10
Copy link
Member Author

This looks generally good. My only question is how does this behave if we have multiple workspaces open each trying to delete the same set of distributions? I'm guessing one of them will throw an error, but continue deleting.

Could you make sure that an error message is not sent to the log if a distribution has already been deleted when the cleaner tries to clean it?

Thanks, that's a good point. I've changed the process to be less likely to all clean-up the same directory by shuffling the array, but I've also added a check for ENOENT which should ensure that when the directory doesn't exist, we don't log the error.

@koesie10 koesie10 requested a review from aeisenberg October 17, 2024 08:50
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Nice!

@koesie10 koesie10 enabled auto-merge October 22, 2024 13:24
@koesie10 koesie10 merged commit b1be37c into main Oct 22, 2024
29 checks passed
@koesie10 koesie10 deleted the koesie10/cleanup-distributions branch October 22, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants