Harden dataset storage cleanup - #12690
Open
ErykKul wants to merge 7 commits into
Open
Conversation
cleanStorage was a GET that deleted by default, so anything following links could trigger it: browser prefetch, link previews, crawlers, or revisiting the URL from history. It is now a PUT, and dryrun defaults to true so omitting the parameter reports instead of deleting. It could also delete real files. Direct upload writes its object to the final storage key before Dataverse registers a DataFile for it, so an upload that had finished but was still waiting to be saved looked exactly like an abandoned one. cleanUp now takes a minimum age and skips anything modified more recently, configurable through dataverse.files.clean-storage-min-age-days, default 7. The listing already retrieved modification times from S3 and discarded them, so the age check costs no extra calls. An unknown timestamp counts as too recent, so a backend that cannot report one never causes a deletion. getToDeleteFilesFilter stays a name-only predicate, leaving its existing test unchanged.
getAuditFiles calls cleanUp purely to list what is in storage, with a match-everything filter and dryRun set. Duration.ZERO keeps that listing complete, which is the behaviour it had before.
|
Coverage is 25.469% — harden-dataset-clean-storage into develop. No base build found for |
This comment has been minimized.
This comment has been minimized.
Sonar failed the gate on duplication and coverage, not on code smells. The three backends had the same filter-and-age pipeline inline, so it moves to StorageIO.selectForCleanUp. That leaves one definition of the selection rule, removes the duplicated block, and makes the part worth testing reachable without a storage backend. Five tests cover it. Also returns an unmodifiable list, drops the two imports that became unused, and removes the unread local in the audit endpoint.
This comment has been minimized.
This comment has been minimized.
FileAccessIOTest already stages files under the default root, so cleanUp can be exercised end to end there: an old orphan is removed, a recently modified one is not, a dry run deletes nothing, and the filter still protects referenced files. That covers the age guard on the backend most installations use. The Swift and S3 equivalents would need test seams in production code, and the resource method needs the container, so both stay with the integration tests.
S3AccessIO already takes an injected client, so its cleanup needs no production change: a mocked listing with per-object timestamps drives it. Swift has no such seam, so swiftContainer drops private to package-private and the test supplies a mocked container. Setting isWriteAccess directly keeps open() out of the way in both. That leaves the resource method and the overlay delegation uncovered, both of which need the container to reach.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
swiftContainer goes back to private and its test comes out. The seam existed only to move the coverage number, and a production visibility change is not worth carrying for that.
Adding selectForCleanUp put it between isOlderThan's javadoc and the method, so the doc described the wrong one.
|
This comment has been minimized.
This comment has been minimized.
1 similar comment
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
cleanStoragedeletes storage objects that are not referenced by any file in the dataset. Three problems with how it does that.It was a
GET, so anything that follows a link could trigger a deletion: browser prefetch, link previews in chat and mail clients, crawlers, or opening the URL again from history. It is now aPUT.dryrundefaulted to false, so omitting the parameter deleted. It now defaults to true, and deleting requiresdryrun=false.It could delete real files. An upload is written to the dataset's storage location before Dataverse registers a
DataFilefor it, so an upload that had finished but was still waiting to be saved looked exactly like an abandoned one and was removed.cleanUpnow takes a minimum age and skips anything modified more recently. The default is 7 days, configurable withdataverse.files.clean-storage-min-age-days.The permission failure also returned 500 instead of 403.
Which issue(s) this PR closes:
Special notes for your reviewer:
The age check costs no extra calls. The S3 listing already retrieved
lastModifiedfor each object and discarded it, and the filesystem listing gets it from the directory walk.An unknown timestamp counts as too recent, so a backend that cannot report one never causes a deletion. That is why the Swift lookup is wrapped.
getToDeleteFilesFilterstays a name-only predicate, so its existing test is unchanged. The age check lives in the storage layer, where the timestamps already are.InputStreamIOandAbstractRemoteOverlayAccessIOonly needed the new signature.This changes the API. Callers have to switch to
PUT, and any that relied on the old default to delete have to passdryrun=false.Suggestions on how to test this:
StorageIOCleanUpAgeTestandDatasetsTest.PUT /api/datasets/{id}/cleanStorage?dryrun=trueand confirm the unsaved upload is not listed.dryrunand confirm the response reports files without deleting them.EditDataseton the dataset gets 403.dataverse.files.clean-storage-min-age-days=0, confirm orphaned files are reported and then deleted withdryrun=false.Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No.
Is there a release notes update needed for this change?:
Yes, included:
doc/release-notes/harden-clean-storage.md.Additional documentation:
Native API guide updated for the new method, the
dryrundefault, and the grace period.