fix(gc): respect in-progress .sync sessions when removing repos

[seniorquico]

What type of PR is this?

bug

Which issue does this PR fix:

#4240

What does this PR do / Why do we need it:

ImageStore.CleanupRepo deletes an entire repository directory when garbage collection finds every committed blob unreferenced. Its "no in-flight writes" guard only checked the .uploads/ directory (ListBlobUploads) and ignored the on-demand sync staging directory .sync/<uuid>/. As a result, GC could remove a repository out from under a live on-demand pull that is staging blobs into it (the default sync config with no downloadDir), corrupting the in-flight sync.

This PR closes that race with two mirrored changes:

1. Guard (pkg/storage/imagestore/imagestore.go): CleanupRepo now also skips repo removal while <repo>/.sync/ contains any session directory, exactly as it already does for a non-empty .uploads/.
2. Reaper (pkg/storage/gc/gc.go): a new removeStaleSyncSessions deletes .sync/<uuid> staging sessions older than the GC delay, mirroring the existing removeBlobUploads reaper. Without it, a session left behind by a crashed/interrupted sync would block repo removal forever (and leak disk). It runs at the end of cleanRepo, after removeUnreferencedBlobs (and thus after the CleanupRepo guard), so a freshly created session's blobs are never reaped out from under it within the same pass.

Known limitation (disclosed): a .sync/<uuid> directory's mtime only advances when its direct children change, so a single download running longer than the GC delay could still be reaped mid-flight. Severity is low — the GC delay defaults to 1 hour (DefaultGCDelay), well above realistic staging times — and this is strictly better than the prior behavior, which deleted live sessions unconditionally via repo removal. A related follow-up is intentionally out of scope: CommitAll (pkg/extensions/sync/destination.go) silently returns nil when its session directory has vanished; it should fail loudly instead.

Testing done on this change:

New unit tests were added covering both halves of the fix, exercised against the real on-disk staging layout (<repo>/.sync/<uuid>/):

• TestCleanupRepoToleratesSyncSessions (pkg/storage/imagestore) — an in-progress .sync session blocks repo removal; removal proceeds when none exists; an empty .sync/ dir does not block removal.
• TestRemoveStaleSyncSessions (pkg/storage/gc) — a stale session (old mtime) is reaped while a fresh one is kept.
• TestRemoveStaleSyncSessionsNoSessions — no .sync directory ⇒ zero deletions, no error.
• TestGCReaperSyncSessionsWithRepoRemoval — a repo with an unreferenced blob plus a stale session: the guard blocks repo removal while the session is present, the blob is GC'd, and the reaper removes the stale session (empty repos are intentionally left in place).

$ go test ./pkg/storage/gc/... ./pkg/storage/imagestore/...
ok      zotregistry.dev/zot/v2/pkg/storage/gc          15.4s
ok      zotregistry.dev/zot/v2/pkg/storage/imagestore  0.1s

$ go test ./pkg/storage/gc/... -run 'TestRemoveStaleSyncSessions|TestGCReaperSyncSessions' -v
--- PASS: TestRemoveStaleSyncSessions
--- PASS: TestRemoveStaleSyncSessionsNoSessions
--- PASS: TestGCReaperSyncSessionsWithRepoRemoval

$ go test ./pkg/storage/imagestore/... -run TestCleanupRepoToleratesSyncSessions -v
--- PASS: TestCleanupRepoToleratesSyncSessions

Will this break upgrades or downgrades?

No. This changes only runtime garbage-collection behavior. There is no on-disk format change, no config schema change, and no API change — the .sync/ staging layout already exists. A binary on either side of this change reads and writes the same storage. Safe to upgrade or downgrade.

Does this PR introduce any user-facing change?:

No API or configuration change. Behaviorally, garbage collection no longer deletes a repository while an on-demand sync is staging into it, and it now reaps stale .sync staging sessions. GC deletion metrics gain one new label value, syncSession, alongside the existing manifest/blob/upload.

Garbage collection no longer removes a repository directory while an on-demand sync is
staging blobs into it, and now reaps stale sync staging sessions (`.sync/<uuid>`) left
behind by interrupted syncs. GC deletion metrics gain a new `syncSession` label value.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[codecov]

Codecov Report

❌ Patch coverage is 60.65574% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.67%. Comparing base (fea81a5) to head (f89b2d5).

Files with missing lines	Patch %	Lines
pkg/storage/gc/gc.go	48.57%	13 Missing and 5 partials ⚠️
pkg/storage/imagestore/imagestore.go	76.92%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4241      +/-   ##
==========================================
- Coverage   91.73%   91.67%   -0.07%     
==========================================
  Files         207      207              
  Lines       30989    31049      +60     
==========================================
+ Hits        28428    28464      +36     
- Misses       1645     1663      +18     
- Partials      916      922       +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

This PR fixes a GC race where ImageStore.CleanupRepo could remove an entire repository directory while an on-demand sync was actively staging blobs under <repo>/.sync/<uuid>/. It extends the “no in-flight writes” guard to include .sync sessions and adds a GC reaper to delete stale .sync sessions after the GC delay.

Changes:

• Extend CleanupRepo’s repo-removal guard to also consider active .sync/<uuid> staging sessions.
• Add removeStaleSyncSessions in GC to reap stale .sync sessions and emit GC deletion metrics with a new syncSession label.
• Add unit tests covering both the new guard behavior and the stale-session reaper behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/test/mocks/image_store_mock.go	Extends the ImageStore mock with sync-session list/stat/delete hooks for GC tests.
pkg/storage/types/types.go	Expands the types.ImageStore interface to include sync-session operations.
pkg/storage/imagestore/imagestore.go	Implements sync-session list/stat/delete and uses sync sessions to block repo removal in CleanupRepo.
pkg/storage/imagestore/imagestore_test.go	Adds a test ensuring .sync sessions prevent repo removal and that an empty .sync does not.
pkg/storage/gc/gc.go	Adds stale .sync session reaping and increments GC metrics for syncSession deletions.
pkg/storage/gc/gc_internal_test.go	Adds tests for reaping stale sync sessions and interaction with repo cleanup behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[andaaron]

Thanks for coming forward with a PR to fix this.

Blocking: sync sessions are always local; the store driver may not be. They are not part of serving storage — only a temporary staging area before the image is validated and pushed into actual storage.

Sync staging always goes through ocidir:// + a local image store (pkg/extensions/sync/oci_layout.go, getImageStore). For remote storage (S3/GCS/Azure), config already requires extensions.sync.downloadDir, so sessions live under that local directory — not under the remote store-driver prefix.

This PR’s ListSyncSessions / StatSyncSession / DeleteSyncSession go through the serving ImageStore’s storeDriver, like .uploads. That only matches the default local layout (no downloadDir), where .sync sits under //.

For remote + downloadDir (and local + downloadDir):

• the repo-wipe race does not apply the same way (remote Delete does not remove the local staging tree);
• the new guard/reaper on the serving store look at the wrong backend;
• orphan cleanup under downloadDir remains unaddressed.

Unlike .uploads (real store-driver objects, including on remote), .sync sessions are not remote objects.
Extending the ImageStore / driver API for them conflates serving storage with always-local ocidir staging and sets the wrong expectation.

I don’t think we should land this as-is. Sync-session lifecycle needs to stay on the local staging root, not on the remote-capable serving store driver. Happy to discuss an approach that keeps the local in-repo guard without putting these APIs on ImageStore.

[seniorquico]

Thanks, @andaaron! Yes, I'm happy to keep working on this with some guidance. What is the best way to workshop an alternative design that guards only the local storage?

[andaaron]

Here's what I propose:

No new ImageStore APIs, drop ListSyncSessions / StatSyncSession / DeleteSyncSession from types.ImageStore. Those belong to local staging, not serving storage.

1. In CleanupRepo, before Delete(repo):

• if the store driver is not local → keep current behavior (remote Delete cannot wipe local downloadDir staging);
• if local → check the filesystem under filepath.Join(RootDir(), repo, syncConstants.SyncBlobUploadDir) with os/filepath (not storeDriver.List);
• if any session dir exists → skip repo removal (same idea as .uploads presence).

2. Add a reaper task generator, but not in GC.

Sync already removes session dirs on the normal path (defer CleanupImage in syncImage, and os.RemoveAll in CommitAll).
Orphans only matter after crash/kill/CleanupImage never running.

If we want cleanup for orphans, a better fit is a separate scheduled task generator in the sync extension that:

• walks known local staging roots (downloadDir if set, else each local store RootDir);
• deletes <repo>/.sync/<uuid>/ sessions older than a delay - I suggest using the same value as for GC delay.
• uses local FS ops only — never the remote store driver.