#2372: Add --retention-delay option to ide cleanup - #2378
#2372: Add --retention-delay option to ide cleanup#2378krystynaShatkovska wants to merge 3 commits into
Conversation
Adds a --retention-delay option to the cleanup commandlet to delete stale files that have not been modified within a configurable period. Files are scanned recursively under $IDE_HOME/updates, $IDE_ROOT/_ide/tmp and ~/Downloads/ide. The option accepts a time-based ISO-8601 duration (e.g. P30D) and defaults to 1 year (365 days) if not provided. Empty folders left behind after deleting stale files are removed, while the scanned roots themselves are kept.
Coverage Report for CI Build 33621501883Coverage increased (+0.09%) to 73.704%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions9 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
samuelkos17
left a comment
samuelkos17
left a comment
There was a problem hiding this comment.
Thanks for adding the --retention-dely to the cleanup commandlet. I tried to follow your testing steps, however the commands you provided didn't work out for me. I've manually moved the files to /_ide/tmp though and then ran the cleanup command and it worked!
While reviewing I found some problems that could lead to issues and that really need to get addressed before moving this to In Review. You can find them in the Comments here.
Besides that I still have on recommendation:
documentation/tmp.adoc line 18 needs to be updated according to the new functionality.
| private Duration getRetentionDelay() { | ||
|
|
||
| String value = this.retentionDelay.getValueAsString(); | ||
| if (value == null) { | ||
| return DEFAULT_RETENTION_DELAY; | ||
| } | ||
| try { | ||
| return Duration.parse(value); | ||
| } catch (DateTimeParseException e) { | ||
| throw new CliException( | ||
| "Invalid value '" + value + "' for --retention-delay. Please provide a time-based ISO-8601 duration such as P30D or PT2H30M.", | ||
| e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Duration.parse() legally accepts negative and zero values. This means it won't throw the CliException and later down the line in isStale() every file under all roots becomes stale, which leads to every file being deleted. You should add a positivity check here.
There was a problem hiding this comment.
You're right that Duration.parse() accepts P0D and -P1D, and with either of them isStale() would flag every file under every root as stale and delete them all. I added a check right after parsing. if the value is zero or negative it now throws a CliException with clear message, so the failure happens up front instead of silently deleting everything. Added a test covering P0D and -P1D
| private void discoverStaleFilesRecursive(Path folder, Duration retentionDelay, List<Path> staleFiles) { | ||
|
|
||
| if (!Files.isDirectory(folder)) { | ||
| return; | ||
| } | ||
|
|
||
| for (Path child : this.context.getFileAccess().listChildren(folder, child -> true)) { | ||
| if (Files.isDirectory(child)) { | ||
| discoverStaleFilesRecursive(child, retentionDelay, staleFiles); | ||
| } else if (isStale(child, retentionDelay)) { | ||
| staleFiles.add(child); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Files.isDirectory(child) follows links. If a symlink inside any root (e.g. ~/Downloads/ide/work -> C:\data) makes the scan descend outside the roots and delete the target's stale files, which would be a huge problem. Furthermore a self-referencing link causes unbounded recursion. You need to check for link children here and skip these. This also destroyed my IDEasy installation and I'm not sure how yours didn't get destroyed when you tested your work.
There was a problem hiding this comment.
tthanks for flagging that it took out your install. Files.isDirectory() follows the link, so the scan would walk into a link target outside the roots (deleting its stale files) and a self-referencing link would loop forever. The scan now skips any child that Files.isSymbolicLink() reports as a link. It neither recurses into it nor deletes it. I added a test also, stale file directly under a scanned root is deleted, but a stale file reached only through a symlinked directory is left alone. That's the exact class of bug that destroyed your setup
| cmd.cleanup=Commandlet to clean up the IDEasy installation by uninstalling all unused tools. | ||
| cmd.cleanup.detail=This will remove any installed tools that are currently not in use by an IDEasy project. Before anything is deleted you are asked for confirmation. Run "ide -b -f cleanup" to skip the confirmation. |
There was a problem hiding this comment.
This needs to be updated to match the new functionality.
| cmd.cleanup=Werkzeug zum Aufräumen der IDEasy-Installation durch Deinstallieren aller ungenutzten Werkzeuge. | ||
| cmd.cleanup.detail=Dies wird alle installierten Werkzeuge entfernen, die derzeit von keinem IDEasy-Projekt verwendet werden. Bevor etwas gelöscht wird, wirst du um Bestätigung gebeten. Führe "ide -b -f cleanup" aus, um die Bestätigung zu überspringen. |
There was a problem hiding this comment.
This needs to be updated to match the new functionality.
| cmd.claude.detail=Claude Code CLI ist ein KI-gestützter Programmierassistent, der über die Befehlszeile ausgeführt wird. Detaillierte Dokumentation ist zu finden unter https://code.claude.com/docs/de/overview | ||
| cmd.cleanup=Werkzeug zum Aufräumen der IDEasy-Installation durch Deinstallieren aller ungenutzten Werkzeuge. | ||
| cmd.cleanup.detail=Dies wird alle installierten Werkzeuge entfernen, die derzeit von keinem IDEasy-Projekt verwendet werden. Bevor etwas gelöscht wird, wirst du um Bestätigung gebeten. Führe "ide -b -f cleanup" aus, um die Bestätigung zu überspringen. | ||
| cmd.cleanup.opt.--retention-delay=die Aufbewahrungsdauer von Dateien in den Ordnern 'updates', '_ide/tmp' und 'Downloads/ide', d.h. Dateien, die innerhalb dieses Zeitraums nicht modifiziert wurden, werden als veraltet gelöscht. Eine zeitbasierte ISO-8601-Dauer (z. B. 'P30D' für 30 Tage oder 'PT2H30M' für 2 Stunden und 30 Minuten). Standardmäßig 1 Jahr (365 Tage), wenn nicht angegeben. |
There was a problem hiding this comment.
| cmd.cleanup.opt.--retention-delay=die Aufbewahrungsdauer von Dateien in den Ordnern 'updates', '_ide/tmp' und 'Downloads/ide', d.h. Dateien, die innerhalb dieses Zeitraums nicht modifiziert wurden, werden als veraltet gelöscht. Eine zeitbasierte ISO-8601-Dauer (z. B. 'P30D' für 30 Tage oder 'PT2H30M' für 2 Stunden und 30 Minuten). Standardmäßig 1 Jahr (365 Tage), wenn nicht angegeben. | |
| cmd.cleanup.opt.--retention-delay=Die Altersgrenze von Dateien in den Ordnern 'updates', '_ide/tmp' und 'Downloads/ide', d.h. Dateien, die innerhalb dieses Zeitraums nicht modifiziert wurden, werden als veraltet gelöscht. Eine zeitbasierte ISO-8601-Dauer (z. B. 'P30D' für 30 Tage oder 'PT2H30M' für 2 Stunden und 30 Minuten). Standardmäßig 1 Jahr (365 Tage), wenn nicht angegeben. |
Furthermore the folders you mention here are correct for Windows and Linux, however on macOS there are somewhere else, maybe just remove the path descriptions and describe the folders?
| if (hasSoftwareToDelete(installedSoftware.getTools())) { | ||
| List<Path> staleRoots = new ArrayList<>(); | ||
| List<Path> staleFiles = new ArrayList<>(); | ||
| if (this.context.getIdeHome() != null) { |
There was a problem hiding this comment.
This if-guard prevents any clean-up from happening, however _ide/tmp and the download cache cleanup would work when IDE_HOME is null. I'm not sure if this intentional, but you might want to change that if it's not.
There was a problem hiding this comment.
It wasn't intentional. Thank you for locating the problem. Guarding the whole stale-file step on getIdeHome() != null meant the _ide/tmp and download-cache roots (which live under IDE_ROOT/user home and are available even outside a project) were silently skipped whenever IDE_HOME was null. I moved the null-check down into getStaleFileRoots(), where only the updates root actually needs it. Now the other two roots are cleaned regardless of IDE_HOME
| cmd.claude.detail=Claude Code CLI is a command-line interface for interacting with the Claude AI assistant. Detailed documentation can be found at https://code.claude.com/docs/en/overview | ||
| cmd.cleanup=Commandlet to clean up the IDEasy installation by uninstalling all unused tools. | ||
| cmd.cleanup.detail=This will remove any installed tools that are currently not in use by an IDEasy project. Before anything is deleted you are asked for confirmation. Run "ide -b -f cleanup" to skip the confirmation. | ||
| cmd.cleanup.opt.--retention-delay=the retention period of files in the 'updates', '_ide/tmp' and 'Downloads/ide' folders, i.e. files that were not modified within this period are deleted as stale. A time-based ISO-8601 duration (e.g. 'P30D' for 30 days or 'PT2H30M' for 2 hours and 30 minutes). Defaults to 1 year (365 days) if not provided. |
There was a problem hiding this comment.
| cmd.cleanup.opt.--retention-delay=the retention period of files in the 'updates', '_ide/tmp' and 'Downloads/ide' folders, i.e. files that were not modified within this period are deleted as stale. A time-based ISO-8601 duration (e.g. 'P30D' for 30 days or 'PT2H30M' for 2 hours and 30 minutes). Defaults to 1 year (365 days) if not provided. | |
| cmd.cleanup.opt.--retention-delay=The retention period of files in the 'updates', '_ide/tmp' and 'Downloads/ide' folders, i.e. files that were not modified within this period are deleted as stale. A time-based ISO-8601 duration (e.g. 'P30D' for 30 days or 'PT2H30M' for 2 hours and 30 minutes). Defaults to 1 year (365 days) if not provided. |
Furthermore the folders you mention here are correct for Windows and Linux, however on macOS there are somewhere else, maybe just remove the path descriptions and describe the folders?
There was a problem hiding this comment.
Updated. I also took your second point, the paths 'updates', '_ide/tmp', 'Downloads/ide' are only correct on Windows and Linux and sit somewhere else on macOS, so I removed the literal paths and describe the folders generically ("the IDEasy updates, temporary and download cache folders"). Same change in the German file. Is it suitable?
| @@ -1,16 +1,22 @@ | |||
| package com.devonfw.tools.ide.commandlet; | |||
|
|
|||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | |||
There was a problem hiding this comment.
| import static org.assertj.core.api.Assertions.assertThatThrownBy; |
dead code
There was a problem hiding this comment.
I double checked this one. I don't think it's actually dead. Line 3 is import static ...assertThatThrownBy and it's used in testCleanupRejectsInvalidRetentionDelay (the assertThatThrownBy(cleanup::run)... call). Removing it would break compilation. If you were pointing at something else that I missed, let me know which line
|
|
||
| LOG.debug("Start cleanup commandlet"); | ||
|
|
||
| Duration retentionDelay = getRetentionDelay(); |
There was a problem hiding this comment.
You might want to rename this since you already have a StringProperty called retentionDelay in the class.
| private void logStaleFilesToBeDeleted(List<Path> staleFiles, Duration retentionDelay) { | ||
|
|
||
| if (staleFiles.isEmpty()) { | ||
| LOG.info("No stale files older than {} will be deleted.", retentionDelay); | ||
| } else { | ||
| for (Path staleFile : staleFiles) { | ||
| LOG.info("\t - {} will be deleted", staleFile); | ||
| } | ||
| LOG.info("Summary: {} stale file(s) older than {} will be deleted.", staleFiles.size(), retentionDelay); | ||
| } | ||
| } |
There was a problem hiding this comment.
You might want to format the duration human-readably.
There was a problem hiding this comment.
Done. Also added a small formatDurationHumanReadable() helper so the summary reads No stale files older than 365 day(s) will be deleted. instead of the raw PT8760H
3 participants
This PR fixes #2372
Implemented changes:
Adds a --retention-delay option to ide cleanup. It takes an ISO-8601 duration (e.g. P30D, PT2H30M; default 1 year) and deletes stale files (not modified within that period) in $IDE_HOME/updates, $IDE_ROOT/_ide/tmp and ~/Downloads/ide. After deletion it deletes empty folders (but keeps the scanned roots). Invalid durations are rejected with a clear error.
Testing instructions
Automated tests
Run the test class, it covers all the core cases:
mvn -pl cli test -Dtest=CleanupCommandletTest
Manual test
The command scans three roots for stale files: $IDE_HOME\updates (only inside a project), $IDE_ROOT_ide\tmp (always exists), and ~\Downloads\ide (the download cache). Use _ide\tmp for the most reliable check since it always exists; the updates example below works too.
a. Create a test file in a scanned folder and backdate it so it's older than the retention delay, plus a recent file in the same folder that should be kept:
a "stale" file older than 30 days
New-Item -Path "$IDE_ROOT_ide\tmp\stale.bin" -Value "x" -Force
(Get-Item "$IDE_ROOT_ide\tmp\stale.bin").LastWriteTime = (Get-Date).AddDays(-31)
a recent file in the same folder that must survive
New-Item -Path "$IDE_ROOT_ide\tmp\fresh.bin" -Value "x" -Force
(Same for $IDE_HOME\updates if you prefer: New-Item -Path "$IDE_HOME\updates\stale.bin" -Value "x" -Force then backdate it the same way.)
b. Run cleanup with a retention delay shorter than the stale file's age, and answer Yes to the "Do you want to continue?" prompt so the deletion actually happens:
ide cleanup --retention-delay=P30D
c. Confirm the stale file is gone and the recent file is kept:
Test-Path "$IDE_ROOT/_ide/tmp/stale.bin" # expected: False
Test-Path "$IDE_ROOT/_ide/tmp/fresh.bin" # expected: True
d. Verify invalid input is rejected (each fails fast with a clear error, before any scanning or deletion):
ide cleanup --retention-delay=PT6M10D # rejected: months aren't allowed in a time-based duration (and segment order is wrong)
ide cleanup --retention-delay=P0D # rejected: must be a positive duration
ide cleanup --retention-delay=-P30D # rejected: must be a positive duration
ide cleanup --retention-delay=abc # rejected: not a valid ISO-8601 duration
ide cleanup --retention-delay=P30D # valid (30 days) — use this for the real run
Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.batand notfeature/921 fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internalpom.xmlfiles or otherwise if runtime dependencies changed, you have updated our LICENSE.asciidoc