-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: Implement cleanup of old AppMap binaries #1009
Conversation
src/assets/assetService.ts
Outdated
await fs.promises.unlink(filePath); | ||
log.info(`Deleted old binary: ${binaries[i].name}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth some sort of assertion that we're not destroying a linked object. These binaries are put in the lib directory, then a symlink is created in the bin directory pointing at the latest binary in lib.
It seems unlikely that this would happen, but if for some reason an asset download failed and the symlink wasn't updated, it may be possible that the linked binary gets deleted here.
src/assets/assetService.ts
Outdated
const result = asset(); | ||
await result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const result = asset(); | |
await result; | |
const result = await asset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good. Please see the requested changes above. Also, the build is failing but it's not clear to me that it's related. Could you look in to it?
72990d0
to
8e2d488
Compare
Changes are done. The integration test is failing, but I re-ran the build of the last release commit, and it seems to be failing as well. This is probably unrelated, but I can try to solve it even if it's unrelated. |
src/assets/assetService.ts
Outdated
const symlinks = (await fs.promises.readdir(binDir, { withFileTypes: true })) | ||
.filter((f) => f.isSymbolicLink()) | ||
.map((f) => join(binDir, f.name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth resolving the linked path once here instead of N times in the loop below (hasSymlink
). It won't change during this time.
8e2d488
to
9fcc2db
Compare
src/assets/assetService.ts
Outdated
// Match patterns like '-v1.2.3', '-v1.2.3-alpha', '-v1.2.3+build', '-1.2.3-alpha+build' | ||
// with or without file extension | ||
const versionMatch = fileName.match( | ||
/-v?(\d+\.\d+\.\d+(-[a-zA-Z0-9-]+)?(\+[a-zA-Z0-9-]+)?)\b(\.[^.]+)?$/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that we are using a recommended RegExp from here - https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string
@@ -94,13 +102,14 @@ export default class AssetService { | |||
sync.emit('error', e); | |||
if (e instanceof AbortError) return reject(e); | |||
} | |||
await this.cleanUpOldBinaries(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trap, log, and ignore any errors that occur here, so that the cleanup doesn't interfere with any other update functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was doing it inside the inner most function AssetService.cleanUpOldBinariesFromDir
, which is called for appmap
, java
and scanner
folders separately. In order to cover symlink resolutions as well, now I moved the try catch logic to AssetService.cleanUpOldBinaries
instead of the call site above, because AssetService.cleanUpOldBinaries
is called from two different places.
src/assets/assetService.ts
Outdated
continue; | ||
} | ||
|
||
await fs.promises.unlink(binaryPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other processes may be concurrently cleaning up the same binaries. Therefore, there is a race condition here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This runs inside execute
method of a new LockfileSynchronizer(appmapDir)
. Doesn't it prevent race condition, since binaryPath
is inside appmapDir
? Or maybe I'm missing something.
c3faa8c
to
f7412c3
Compare
4c53c5b
to
f7412c3
Compare
- Added 'cleanUpOldBinaries' and 'cleanUpOldBinariesFromDir' functions to handle the removal of older binaries. - Integrated 'cleanUpOldBinaries' to run after downloading a binary. - Added a unit test to verify the cleanup process. This change addresses storage bloat by ensuring that only the latest versions of binaries are kept, enhancing the efficiency and storage management of the AppMap plugin.
7e31c09
to
d9747f2
Compare
d9747f2
to
f76a289
Compare
Closing this for now. The plan is to instead implement this within the appmap CLI. |
Fixes #1007.
This change addresses storage bloat by ensuring that only the latest versions of binaries are kept, enhancing the efficiency and storage management of the AppMap plugin.
Problem
The AppMap plugin currently does not clean up older versions of AppMap binaries located in the
$HOME/.appmap
(Linux/macOS) or%HOME%/.appmap
(Windows) directory. This can lead to unnecessary storage usage as multiple old versions accumulate over time. The goal is to remove all older versions of the binaries and ensure that only the most recent version is retained moving forward.Analysis
To resolve this issue, we need to implement a cleanup function that scans the AppMap binaries directory and deletes all but the most recent binary. The most recent binary can be determined by the
semver
version number in the file name.Solution
This PR introduces:
$HOME/.appmap
directory.Code Changes
1. Directory Management and Utility Enhancements
getAppmapDir
to centralize the base directory path for.appmap
.2. Cleanup Logic Implementation
cleanUpOldBinaries
andcleanUpOldBinariesFromDir
to handle the removal of older binaries.3. Integration
cleanUpOldBinaries
to run after downloading a binary.Modifications:
4. Testing
Test Modifications:
PR Summary
This PR addresses storage bloat by implementing cleanup functionality for old binaries in the AppMap plugin. This ensures that only the latest versions of binaries are kept, enhancing the efficiency and storage management of the AppMap plugin. This update is crucial to maintain optimal performance and prevent unnecessary storage consumption.