Skip to content

enhance(packages): add Maven snapshot build retention - #33420

Open
dianaStr7 wants to merge 4 commits into
go-gitea:mainfrom
dianaStr7:main
Open

dianaStr7 wants to merge 4 commits into
go-gitea:mainfrom
dianaStr7:main

Conversation

@dianaStr7

Copy link
Copy Markdown
Contributor

Pull Request: Implement Cleanup Function for Maven Snapshot Versions

Overview

This pull request introduces a cleanup function for Maven snapshot versions in Gitea, enabling more efficient management of package storage. The new feature allows users to specify how many of the most recent Maven snapshot builds to retain, optimizing storage by automatically removing older files.

Features

  • New Configurable Variable: RETAIN_MAVEN_SNAPSHOT_BUILDS allows setting the number of Maven snapshot builds to keep.
    • Default is -1, which keeps all builds.
  • Automated Cleanup: Tied to the cleanup expired data process, this function targets files within Maven snapshots, not the versions themselves, ensuring that only essential files are kept.

Implementation

The feature extends the existing cleanup job to include files from Maven snapshot versions based on the specified retention policy set in app.ini. It checks against the highest build number from the Maven metadata file to determine which files to retain.

Impact

This enhancement helps manage disk space more effectively by providing control over how many builds of Maven snapshots are retained, potentially reducing storage requirements for projects using Maven within Gitea.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 27, 2025
@github-actions github-actions Bot added modifies/go docs-update-needed The document needs to be updated synchronously labels Jan 27, 2025
@dianaStr7

Copy link
Copy Markdown
Contributor Author

Considered Alternative Approach

I considered another way to handle Maven snapshot cleanup that uses the properties field for package files to store build numbers right when files are uploaded. This would make database searches simpler.

This method would need a database migration to work, so it's a bit more complicated. I have the code ready for this alternative if we think it's worth the extra steps later on.

@delvh delvh changed the title Added cleanup method for files in Maven snapshot versions Add time-based cleanup for Maven snapshot versions Jan 31, 2025
@delvh

delvh commented Jan 31, 2025

Copy link
Copy Markdown
Member

Note to any reviewer: Review this PR carefully.
It seems like an LLM at the very least helped with creating this PR.
That in and of itself is nothing bad, as long as the output is indeed correct.
Given my experience with ChatGPT, I find it rather likely that it missed some edge cases though.

@dianaStr7

Copy link
Copy Markdown
Contributor Author

A remark: Yes, it's true that I used ChatGPT for help since I'm not an experienced Go developer, but I handled the core logic and testing myself. I'd appreciate any comments or critiques to help us improve this, as we really need the feature :)

Note to any reviewer: Review this PR carefully. It seems like an LLM at the very least helped with creating this PR. That in and of itself is nothing bad, as long as the output is indeed correct. Given my experience with ChatGPT, I find it rather likely that it missed some edge cases though.

Comment thread modules/setting/packages.go
Comment thread modules/packages/maven/metadata.go Outdated
Comment thread modules/packages/maven/metadata.go Outdated
}

// ParseMavenMetadata parses the Maven metadata XML to extract the build number.
func ParseMavenMetaData(r io.Reader) (string, error) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am thoroughly confused by this method signature and docs: Nothing indicates to me that this method parses the entire XML, and throws the result except for the build number away.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've changed the signature and added a new snapshotMetadata type which can be extended later, does it make it more readable?

Comment thread models/packages/package_file.go Outdated
Comment thread models/packages/package_file.go Outdated
Comment thread services/packages/maven/cleanup.go Outdated
Comment thread models/packages/package_file.go Outdated
Comment thread models/packages/package_file.go Outdated
Comment thread services/packages/maven/cleanup.go Outdated
Comment thread services/packages/maven/cleanup.go Outdated
@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 20, 2025
@delvh
delvh self-requested a review March 8, 2025 17:46
@dianaStr7

dianaStr7 commented Jun 6, 2025 •

Copy link
Copy Markdown
Contributor Author

The feature is tested, but few adjustments need to be done:

  1. When the cleanup job tries to remove a very large number of files, UI times out and nginx responds with 502 Bad Gateway. We should either paginate the requests or hand the work off to an asynchronous background task and stream status updates to the UI?
  2. Need a better approach with artifacts to delete in batches, now it's all gathered in one slice and then deleted one by one.
  3. Can we somehow use parallelization?
  4. Important one: as older artifacts are getting deleted, there is a chance that maven-metadata.xml will show the artifacts which are not present anymore. For that we need to introduce another maven-metadata cleanup. WIt can be dedicated cron job that periodically rebuilds metadata for every groupId/artifactId snapdhot version. During package cleanup we’d call the same helper for just-deleted versions so the main job doesn’t get any longer.
  5. Add a dry run variable?
  6. Unrelated to this pull request, but to packages cleanup: If you want to delete the package version older then 6 month, it uses creation time of the version, not last updated. For maven snapshot versions it can be problematic as they can be in development for a few month, so the creation date is not relevant that much.

Will be happy to hear any suggestions!
@delvh

@proxity

proxity commented Sep 9, 2025 •

Copy link
Copy Markdown

All considerable maven repositories offer a clean-up function, since it's common to have a lot of builds using a snapshot version, until the new version is finished. Therefore, it's essential to get this PR merged now @delvh and @lunny. Otherwise, snapshot versions pile up and use a lot of unnecessary disk space.

@lunny

lunny commented Sep 9, 2025

Copy link
Copy Markdown
Member

Some adjustments are still required to get the CI passing.

@dianaStr7

Copy link
Copy Markdown
Contributor Author

The test for cleanup snapshot versions and code are working, the tests are failing due to:

services/packages/cleanup/cleanup.go:28:6: exported: func name will be used as cleanup.CleanupTask by other packages, and that stutters; consider calling this Task (revive)
func CleanupTask(ctx context.Context, olderThan time.Duration) error {
     ^
services/packages/cleanup/cleanup.go:168:6: exported: func name will be used as cleanup.CleanupExpiredData by other packages, and that stutters; consider calling this ExpiredData (revive)
func CleanupExpiredData(ctx context.Context, olderThan time.Duration)

@lunny as this is code was introduced earlier, should I still adapt it to make the tests pass?

@lunny

lunny commented Jan 14, 2026

Copy link
Copy Markdown
Member

The test for cleanup snapshot versions and code are working, the tests are failing due to:

services/packages/cleanup/cleanup.go:28:6: exported: func name will be used as cleanup.CleanupTask by other packages, and that stutters; consider calling this Task (revive)
func CleanupTask(ctx context.Context, olderThan time.Duration) error {
     ^
services/packages/cleanup/cleanup.go:168:6: exported: func name will be used as cleanup.CleanupExpiredData by other packages, and that stutters; consider calling this ExpiredData (revive)
func CleanupExpiredData(ctx context.Context, olderThan time.Duration)

@lunny as this is code was introduced earlier, should I still adapt it to make the tests pass?

Yes. We have to fix all the lint errors before merge.

@dianaStr7
dianaStr7 force-pushed the main branch 2 times, most recently from 50c605b to adbe41c Compare March 20, 2026 13:29
@HoffmannTom

Copy link
Copy Markdown

This feature would be very helpful and save a lot of space, reducing snapshot sizes by around a factor of 5.
The failing checks seem to be related only to the linter, especially regarding method naming.
@dianaStr7 Can the linter issues be fixed? I think your work is close to being finished
Thanks to all!

@dianaStr7

Copy link
Copy Markdown
Contributor Author

This feature would be very helpful and save a lot of space, reducing snapshot sizes by around a factor of 5. The failing checks seem to be related only to the linter, especially regarding method naming. @dianaStr7 Can the linter issues be fixed? I think your work is close to being finished Thanks to all!

Hi @HoffmannTom ! Thank you for the interest :) Yes, I'll solve lint problem in the next commit, just need a bit more testing time as it will be bundled with "rebuild metadata" feature.

@HoffmannTom

Copy link
Copy Markdown

Hi @dianaStr7
Thanks for the update :) Take your time for testing.
Deleting and rebuilding the meta files is for sore a critical aspect.

Ein Servus nach Minga 👋

@bircni
bircni self-requested a review April 19, 2026 13:13
@lunny lunny removed the modifies/go label Apr 19, 2026
@dianaStr7
dianaStr7 force-pushed the main branch 2 times, most recently from 4666eff to 0443d70 Compare July 15, 2026 13:42
@bircni bircni changed the title Add time-based cleanup for Maven snapshot versions feat: Add time-based cleanup for Maven snapshot versions Jul 15, 2026
@bircni bircni changed the title feat: Add time-based cleanup for Maven snapshot versions enhance: Add time-based cleanup for Maven snapshot versions Jul 15, 2026
@github-actions github-actions Bot added the type/enhancement An improvement of existing functionality label Jul 15, 2026
Add configurable retention and dry-run settings for Maven snapshot artifacts.
@proxity

proxity commented Sep 28, 2026

Copy link
Copy Markdown

I have to apply this PR since 1.5 years to all new gitea versions. It runs perfectly. Any chance to merge it now @lunny?

@tschoellhorn

Copy link
Copy Markdown

Yes - please!

@bircni

bircni commented Sep 28, 2026

Copy link
Copy Markdown
Member

ping @delvh

Move Maven specific parsing out of models, run the cleanup outside the
shared cleanup transaction, derive retained builds from stored files,
protect files referenced by maven-metadata.xml including signatures and
drop the DEBUG_MAVEN_CLEANUP setting.
@bircni

bircni commented Sep 28, 2026

Copy link
Copy Markdown
Member

I pushed a cleanup of the snapshot retention logic:

  • Maven-specific parsing moved out of models/packages into modules/packages/maven, and the selection logic is in services/packages/maven.
  • The cleanup now runs before the shared CleanupExpiredData transaction, with one transaction per version. Previously, a single failing version rolled back container and blob cleanup as well. Failures are now logged and that version is skipped.
  • The RETAIN_MAVEN_SNAPSHOT_BUILDS newest builds are taken from the stored file names, not from <buildNumber> in maven-metadata.xml. That file isn't validated on upload, so an inflated value could delete real builds. Gaps in build numbers no longer reduce how many builds are kept.
  • Files listed in maven-metadata.xml are always kept, including their signature files (.asc etc.). File names are matched using the artifact ID from the pom, not from the metadata XML.
  • Removed DEBUG_MAVEN_CLEANUP. It defaulted to true, so enabling retention alone silently did nothing. Cleanup is still opt-in, because the default -1 keeps all builds.
  • Snapshot versions are now filtered in SQL.
  • The tests are consolidated into a parser unit test and a single cleanup test with a table of files to keep and delete.

@bircni
bircni requested a review from silverwind September 28, 2026 19:23
@bircni bircni changed the title enhance: Add time-based cleanup for Maven snapshot versions enhance(packages): add Maven snapshot build retention Sep 28, 2026
}

func CleanupExpiredData(ctx context.Context, olderThan time.Duration) error {
if err := maven_service.CleanupSnapshotVersions(ctx); err != nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm… Should that be a part of the overall transaction, or should it be a separate command?
No idea which is better.

Comment on lines +45 to +46
{"test-project-1.0-20230101.000000-3-sources.jar", true},
{"test-project-1.0-20230101.000000-3-sources.jar.asc", true},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wait, why are the sources kept?
Shouldn't they be deleted too?


pvs, _, err := packages_model.SearchVersions(ctx, &packages_model.PackageSearchOptions{
Type: packages_model.TypeMaven,
Version: packages_model.SearchValue{Value: "%-snapshot"},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the version case-insensitive?

Comment on lines +95 to +98
return !ok || build > threshold || slices.ContainsFunc(referenced, func(v maven_module.SnapshotVersion) bool {
name := v.FileName(metadata.ArtifactID)
return pf.Name == name || strings.HasPrefix(pf.Name, name+".") // keep signatures like .asc
})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't quite understand this retain.
Conceptually, the retain condition is ok && build < threshold, right?
Why would we want to keep signatures? And what's up with the filename check?

@bircni bircni added this to the 29.0.0 milestone Sep 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-update-needed The document needs to be updated synchronously lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged topic/packages type/enhancement An improvement of existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants