Skip to content

feat(gc): reclaim uploads a client never came back for - #123

Merged
BryanFRD merged 1 commit into
mainfrom
feat/reclaim-abandoned-uploads
Aug 17, 2026
Merged

feat(gc): reclaim uploads a client never came back for#123
BryanFRD merged 1 commit into
mainfrom
feat/reclaim-abandoned-uploads

Conversation

@BryanFRD

Copy link
Copy Markdown
Contributor

Closes #122, which #121 created.

A client can negotiate, PUT the object straight to the bucket, and never report it. The bytes sit under its own upload key, nothing else will ever look at them, and nothing removed them. The local path has had a reclaimer for exactly this since the beginning; the bucket had none. Small per event, unbounded over time, and invisible, which is the combination that fills a disk without anyone knowing why.

Swept on the same schedule and by the same setting as an interrupted transfer's staging file: once at boot, then hourly, for anything older than LFSX_STAGING_MAX_AGE. Reusing the setting rather than inventing a second one, because there should be one answer to "when does abandoned work go away".

A key written a moment ago is left alone. A slow client on a bad connection is not an abandoned one, and sweeping it would turn a long push into a failed one. There is a test for each direction, and the abandoned one fails if the bucket sweep is skipped.

One thing it fixed on the way

main.rs built its own LocalStore from the storage root for the reclaimer, while app() built the real store separately. That was fine while the only thing to reclaim was a file on the volume, and it would have quietly swept half a bucket deployment. Both now go through one backends(&config), so the reclaimer cannot come to disagree with the server about where objects live.

A timestamp it cannot read is treated as too young

The age comes from the listing's LastModified. If that cannot be parsed the key is left alone, because deleting somebody's upload on the strength of a date this server did not understand is the wrong way to be wrong. Same rule as a lock with an unreadable timestamp.

242 tests, twenty of them against a real MinIO, clippy clean.

Copilot AI lite review requested due to automatic review settings August 17, 2026 11:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@BryanFRD
BryanFRD enabled auto-merge (squash) August 17, 2026 11:42
@github-actions

Copy link
Copy Markdown

SonarQube — aucune nouvelle issue

Comparaison entre le projet bac à sable de cette PR et la branche par défaut : SonarQube Community n'analyse pas les PR, ce delta est calculé côté CI. Détail

@ferrfleet ferrfleet Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Solid feature — reusing LFSX_STAGING_MAX_AGE, treating an unparsable LastModified as "too young", and having a test for both directions (swept vs. left alone) are all the right calls. One issue worth fixing before merge.

Blocking: server/src/lib.rsreclaim(config) now calls backends(config) to get the store, and main.rs's reclaim_periodically calls lfsx_server::reclaim(&config) every hour forever. backends() unconditionally runs the bucket setup path, including the tracing::warn! calls ("objects and locks are stored in a bucket: collection, deduplication...", the LFSX_S3_PRESIGN warning, etc.) and — if LFSX_ENCRYPTION_KEY_FILE is set — reloads and re-validates the keyring from disk. For every bucket-backed deployment (the exact audience this PR targets), those one-time startup warnings will now be re-emitted at WARN level every hour indefinitely, which reads as a live problem to anyone alerting on WARN-level logs and is pure noise otherwise. It's also wasteful: a fresh S3Store/reqwest::Client and LocalStore get built and thrown away on every tick just to reclaim.

This spans lib.rs and main.rs, and there's more than one reasonable fix, so a suggestion block doesn't fit cleanly. I'd build the Store once (in main, or by having app() return it alongside the Router) and hand that same handle to reclaim_periodically/the boot-time reclaim call, rather than reconstructing the whole backend — including its warnings — from Config on every sweep.

Nit: server/src/storage/staging.rs still exports a free pub async fn reclaim(root, older_than) that only sweeps local staging files. Nothing calls it anymore now that main.rs goes through lfsx_server::reclaim, and it's easy to mistake for the new bucket-aware version later. Worth deleting along with its pub use in storage/mod.rs unless something outside this diff still depends on it.

@BryanFRD
BryanFRD merged commit f9f886b into main Aug 17, 2026
23 checks passed
@BryanFRD
BryanFRD deleted the feat/reclaim-abandoned-uploads branch August 17, 2026 12:37
@BryanFRD BryanFRD mentioned this pull request Aug 17, 2026
4 tasks
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.

An abandoned pre-signed upload leaves its bytes in the bucket forever

2 participants