feat: restrict periodic storage GC to a configurable gcTimeWindow by shcherbak · Pull Request #4223 · project-zot/zot

shcherbak · 2026-07-13T09:48:36Z

Adds a gcTimeWindow storage config option (e.g. "01:00-08:00") that restricts periodic garbage collection to a daily off-peak window, avoiding lock contention on storage during high-load periods. Resolves #3001.

New gcTimeWindow field on StorageConfig (top-level and per-subpath), start inclusive / end exclusive, wraps past midnight (e.g. "22:00-06:00"). Applied at server startup, same as the existing
gcDelay/gcInterval settings - a config file reload updates the stored value but doesn't affect already-running periodic GC tasks; a restart is required to pick up changes.
gc.ValidateGCTimeWindow validates the "HH:MM-HH:MM" format at config load time; windows that wrap past midnight (e.g. "22:00-06:00") are supported.
GCTaskGenerator.IsReady now also checks the configured window before allowing a GC run to proceed.
Unit tests for parsing, validation, wrap-around containment, and generator behavior; config-load tests for valid/invalid windows and the case where a window is set without GC enabled.
Docs and example config updated (examples/README.md, examples/config-gc.json).

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

Copilot

Pull request overview

Adds a configurable daily GC time window (gcTimeWindow, e.g. 01:00-08:00) to restrict periodic storage garbage collection to off-peak hours, reducing lock contention on busy registries.

Changes:

Introduces parsing/validation for "HH:MM-HH:MM" windows (including wrap-around past midnight) and enforces the window in GCTaskGenerator.IsReady().
Wires the new config field through API config, controller GC task setup, and CLI config validation (including per-subpath).
Adds unit/config-load tests plus documentation and example config updates.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
pkg/storage/gc/gc.go	Adds time window parsing/validation helpers and enforces the window in periodic GC readiness.
pkg/storage/gc/gc_internal_test.go	Adds tests for parsing/validation/containment and generator readiness behavior.
pkg/cli/server/root.go	Validates `gcTimeWindow` at config load (top-level + subpaths) and warns if GC disabled.
pkg/cli/server/root_test.go	Adds config-load tests for valid/invalid windows and GC-disabled-with-window case.
pkg/api/controller.go	Passes `gcTimeWindow` into GC options when scheduling periodic GC tasks.
pkg/api/config/config.go	Adds `GCTimeWindow` to `StorageConfig` and includes it in reloadable config updates.
examples/README.md	Documents `gcTimeWindow` usage and wrap-around behavior.
examples/config-gc.json	Adds `gcTimeWindow` to the GC example config.

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

codecov · 2026-07-13T10:11:47Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.76%. Comparing base (fea81a5) to head (96973cd).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4223      +/-   ##
==========================================
+ Coverage   91.73%   91.76%   +0.03%     
==========================================
  Files         207      207              
  Lines       30989    31065      +76     
==========================================
+ Hits        28428    28508      +80     
+ Misses       1645     1642       -3     
+ Partials      916      915       -1

☔ 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

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

rchincha · 2026-07-13T17:20:47Z

The downside of this approach is that the gc work is no longer amortized but has to finish within that time window - which it may not.

shcherbak · 2026-07-13T19:23:01Z

The downside of this approach is that the gc work is no longer amortized but has to finish within that time window - which it may not.

Good catch - updated the design so gcTimeWindow only gates when a new sweep is allowed to start, not every repo-task within it. Once a sweep begins inside the window it runs to completion even past the window's end, so GC work stays fully amortized and won't stall mid-sweep on a large registry until the next day's window. Docs and tests updated accordingly.

andaaron · 2026-07-13T20:52:58Z

+		logger.Error().Err(err).Str("gcTimeWindow", storageConfig.GCTimeWindow).
+			Msg("invalid garbage-collect time window specified")
+
+		return err


Suggested change

return err

return fmt.Errorf("%w: invalid garbage-collect time window specified %s",

zerr.ErrBadConfig, storageConfig.GCTimeWindow)

Same for substores.

This comment is still valid.

andaaron · 2026-07-13T21:12:10Z

-	CacheDriver     map[string]any `mapstructure:",omitempty"`
+	// GCTimeWindow restricts periodic garbage-collection runs to a daily time-of-day
+	// window, e.g. "01:00-08:00". Empty means GC can run at any time.
+	GCTimeWindow  string


GCTimeWindow is a raw string parsed again in validateGC and again in newGCTimeWindow.
How about making this a typed field (e.g. gc.TimeWindow) decoded at LoadConfiguration time via a mapstructure decode hook alongside StringToTimeDurationHookFunc (root.go:1261-1263). That matches how gcDelay/gcInterval work and avoids double-parsing.

Good catch - agreed the double-parse isn't ideal. Only wrinkle: gc already imports config (for ImageRetention), so a gc.TimeWindow type would create an import cycle. Fix would be defining the typed window in config instead (like ImageRetention already is), decoded via a custom mapstructure hook next to StringToTimeDurationHookFunc, with gc.Options.TimeWindow taking that type directly and parseGCTimeWindow/newGCTimeWindow/ValidateGCTimeWindow going away. That's a decent-sized change for this PR's scope. Do we want it here, or as a follow-up?

What I meant is defining a GCTimeWindow struct in this package (e.g. in config.go), with a custom mapstructure decode hook in the same package — similar in spirit to SinkConfigDecoderHook in pkg/extensions/config/events/decoder.go.

Register that hook in root.go next to StringToTimeDurationHookFunc(). Move parsing/validation (currently in parseGCTimeWindow, Contains, empty string = no restriction) into pkg/api/config; invalid values would then fail at unmarshal time.

gc.Options.TimeWindow would take config.GCTimeWindow directly (like it already takes config.ImageRetention), so pkg/storage/gc would not re-parse or validate — it would only consume the already-decoded value.

This should be cleaner and require no extra packages to import in root.go or gc.go

andaaron · 2026-07-13T21:14:10Z

+	// inside the window is allowed to run to completion even past the window's end,
+	// so GC work stays amortized and orphaned blobs don't outpace a narrow window.
+	// Empty means no restriction.
+	TimeWindow string


Once config decodes to a typed window, pass it through Options directly instead of re-parsing the string in newGCTimeWindow (:124-137).

Same suggestion as the one on config.go:39-41

See my other comment in config.go.

andaaron · 2026-07-13T21:15:10Z

+		log.Error().Err(err).Str("gcTimeWindow", window).
+			Msg("invalid gcTimeWindow, ignoring and running GC without a time restriction")
+
+		return nil


After config-load validation, newGCTimeWindow's "log and ignore invalid window" path is likely unreachable in normal operation.

Agree - with the current call graph it's unreachable, since validateGC already rejects the config before the server ever calls this. Kept it as a safety net rather than removing it: gc is a library package with an exported Options.TimeWindow string, and if a direct caller skips validation, silently trusting the parse would leave a zero-value window (startMin=0, endMin=0), which makes contains() always false - GC would stop running permanently and silently, which is worse than the current logged fallback. Added a comment explaining why it's there.

Signed-off-by: shcherbak <ju.shcherbak@gmail.com>

- clarify in docs that gcTimeWindow is start-inclusive/end-exclusive and only applied at server startup, not on config reload - read time.Now() once in GCTaskGenerator.IsReady to avoid an inconsistent readiness check across a minute boundary - fix a flaky test where the "inside window" case excluded the last minute of the day (23:59) Signed-off-by: shcherbak <ju.shcherbak@gmail.com>

…t, and add missing test coverage Signed-off-by: shcherbak <ju.shcherbak@gmail.com>

Signed-off-by: shcherbak <ju.shcherbak@gmail.com>

andaaron · 2026-07-14T09:14:40Z

@shcherbak I forgot to ask: what timezone is this supposed to be in? I think it would be very ambiguous for the user as well

Copilot

Pull request overview

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

+		Convey("inside the window, generator is ready", func() {
+			insideWindow := gcTimeWindow{
+				startMin: 0,
+				endMin:   24 * minutesInHour,
+			}
+
+			gen := &GCTaskGenerator{timeWindow: &insideWindow}
+			So(gen.IsReady(), ShouldBeTrue)
+		})


…nst re-parsing/nil-logger panics

…al time

shcherbak · 2026-07-14T22:29:31Z

@shcherbak I forgot to ask: what timezone is this supposed to be in? I think it would be very ambiguous for the user as well

Fixed: the window is now explicitly evaluated in UTC. Docs and comments updated to say UTC explicitly.

thees · 2026-07-15T06:45:07Z

The downside of this approach is that the gc work is no longer amortized but has to finish within that time window - which it may not.

I created this fix to speed up gc massively: #4236

rchincha · 2026-07-16T23:33:49Z

@shcherbak pls take care of the review comments and ci failures.

We can include this PR in the next release.

andaaron · 2026-07-17T06:00:21Z

What I meant is defining a GCTimeWindow struct in this package (e.g. in config.go), with a custom mapstructure decode hook in the same package — similar in spirit to SinkConfigDecoderHook in pkg/extensions/config/events/decoder.go.

Register that hook in root.go next to StringToTimeDurationHookFunc(). Move parsing/validation (currently in parseGCTimeWindow, Contains, empty string = no restriction) into pkg/api/config; invalid values would then fail at unmarshal time.

gc.Options.TimeWindow would take config.GCTimeWindow directly (like it already takes config.ImageRetention), so pkg/storage/gc would not re-parse or validate — it would only consume the already-decoded value.

This should be cleaner and require no extra packages to import in root.go or gc.go

Please look into this

rchincha · 2026-07-17T06:01:23Z

+}
+
+// ValidateGCTimeWindow returns an error if window is non-empty and not a valid
+// "HH:MM-HH:MM" daily time-of-day range. An empty window is valid and means no restriction.


00:00-00:00?

rchincha · 2026-07-17T06:01:44Z

        "gcDelay": "2h",
-        "gcInterval": "1h"
+        "gcInterval": "1h",
+        "gcTimeWindow": "01:00-08:00"


Not including this value means it defaults to a 24h window?

shcherbak requested review from andaaron and rchincha as code owners July 13, 2026 09:48

andaaron requested a review from Copilot July 13, 2026 09:50

Copilot started reviewing on behalf of andaaron July 13, 2026 09:51 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Comment thread pkg/storage/gc/gc.go

Comment thread pkg/storage/gc/gc_internal_test.go

Comment thread pkg/api/controller.go

Comment thread examples/README.md Outdated

andaaron requested a review from Copilot July 13, 2026 13:22

Copilot started reviewing on behalf of andaaron July 13, 2026 13:22 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Comment thread pkg/storage/gc/gc.go

Comment thread pkg/storage/gc/gc_internal_test.go

Comment thread pkg/api/controller.go

andaaron requested a review from Copilot July 13, 2026 14:11

Copilot started reviewing on behalf of andaaron July 13, 2026 14:12 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Comment thread pkg/cli/server/root.go Outdated

andaaron reviewed Jul 13, 2026

View reviewed changes

shcherbak added 4 commits July 14, 2026 11:37

feat: gcTimeWindow for storage GC only during off-peak hours

9dd85da

Signed-off-by: shcherbak <ju.shcherbak@gmail.com>

fix(gc): amortize periodic sweeps across gcTimeWindow, fix err113 lin…

c8d6b3a

…t, and add missing test coverage Signed-off-by: shcherbak <ju.shcherbak@gmail.com>

fix(gc): skip gcTimeWindow validation when subpath GC is disabled

bedc661

Signed-off-by: shcherbak <ju.shcherbak@gmail.com>

shcherbak force-pushed the feat-gcTimeWindow branch from 403548b to bedc661 Compare July 14, 2026 08:47

andaaron requested a review from Copilot July 14, 2026 09:13

Copilot started reviewing on behalf of andaaron July 14, 2026 09:14 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

shcherbak added 2 commits July 15, 2026 00:21

fix(gc): log GC sweep deferral once per window episode and guard agai…

400cd48

…nst re-parsing/nil-logger panics

fix(gc): evaluate gcTimeWindow in UTC instead of ambiguous server loc…

ebd4ff8

…al time

rchincha added this to the v2.1.19 milestone Jul 16, 2026

rchincha reviewed Jul 17, 2026

View reviewed changes

Merge branch 'main' into feat-gcTimeWindow

96973cd

	return err
	return fmt.Errorf("%w: invalid garbage-collect time window specified %s",
	zerr.ErrBadConfig, storageConfig.GCTimeWindow)

Uh oh!

Conversation

shcherbak commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

rchincha commented Jul 13, 2026

Uh oh!

shcherbak commented Jul 13, 2026

Uh oh!

andaaron Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andaaron commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

shcherbak commented Jul 14, 2026

Uh oh!

thees commented Jul 15, 2026

Uh oh!

rchincha commented Jul 16, 2026

Uh oh!

andaaron commented Jul 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

shcherbak commented Jul 13, 2026 •

edited

Loading

codecov Bot commented Jul 13, 2026 •

edited

Loading

andaaron Jul 13, 2026 •

edited

Loading

andaaron commented Jul 14, 2026 •

edited

Loading