From 0aa274944e8af285cf64be53b7f2c0b534d97ed2 Mon Sep 17 00:00:00 2001 From: bc Date: Sun, 15 Nov 2026 06:42:23 +0100 Subject: [PATCH 1/3] Warn when a top-level cooldown duration can't be parsed - Report unparsable cooldown.default, ecosystems, packages and package_patterns values in Config.Warnings, which are logged at startup - Warn instead of failing so existing configs keep starting; the cooldown library reads such values as 0, which disabled cooldown silently - Document the accepted duration units - Add tests for invalid, valid and environment-supplied durations --- docs/configuration.md | 2 +- internal/config/config.go | 2 +- internal/config/cooldown_profiles.go | 13 +++++ internal/config/cooldown_profiles_test.go | 58 +++++++++++++++++++++++ internal/config/mirror.go | 3 ++ 5 files changed, 76 insertions(+), 2 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 68bb79c1..4ac2806b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1275,7 +1275,7 @@ cooldown: | `cooldown.packages` | - | Per-package overrides (keyed by PURL) | | `cooldown.package_patterns` | - | Per-package glob overrides (keyed by PURL glob) | -Durations support days (`7d`), hours (`48h`), and minutes (`30m`). Set to `0` to disable. +Durations support days (`7d`), hours (`48h`), and minutes (`30m`). Set to `0` to disable. Other units such as weeks (`1w`) are not supported: a top-level value that can't be parsed applies no cooldown, so the proxy logs a warning at startup naming it. Invalid durations inside `cooldown.profiles` are rejected at startup. Package PURL keys are normalized to canonical form before matching, so `pkg:npm/@babel/core` and `pkg:npm/%40babel/core` are equivalent, as are `pkg:pypi/Django` and `pkg:pypi/django`. If both forms configure the same package, the canonical entry wins. diff --git a/internal/config/config.go b/internal/config/config.go index 0cfaa3b8..abba89b0 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -288,7 +288,7 @@ func (c *CooldownConfig) NormalizedPackages() map[string]string { // Shortest returns the shortest cooldown configured anywhere, by default, // ecosystem, package or package pattern, or 0 when none is. Unparsable -// durations are left out; Validate reports them. +// durations are left out; Config.Warnings reports them. func (c *CooldownConfig) Shortest() time.Duration { if c == nil { return 0 diff --git a/internal/config/cooldown_profiles.go b/internal/config/cooldown_profiles.go index 225dd201..95752621 100644 --- a/internal/config/cooldown_profiles.go +++ b/internal/config/cooldown_profiles.go @@ -18,6 +18,19 @@ func (c *CooldownConfig) Policy() (*cooldown.Policy, error) { }, c.PackagePatterns) } +// durationWarning describes a top-level cooldown duration that cannot be +// parsed, or returns "" when they all parse. +// +// The cooldown library reads a duration it cannot parse as 0, so a typo such +// as "1w" turns cooldown off without notice. Profiles already reject such +// values; the top level only warns, so configs that start today keep starting. +func (c *CooldownConfig) durationWarning() string { + if err := c.validateProfileDurations(); err != nil { + return fmt.Sprintf("cooldown: %v; no cooldown applies where this value is used", err) + } + return "" +} + // ValidateProfiles checks names and policies before they become routes. func (c *CooldownConfig) ValidateProfiles() error { for _, name := range slices.Sorted(maps.Keys(c.Profiles)) { diff --git a/internal/config/cooldown_profiles_test.go b/internal/config/cooldown_profiles_test.go index 6f10362d..7b9a9893 100644 --- a/internal/config/cooldown_profiles_test.go +++ b/internal/config/cooldown_profiles_test.go @@ -79,3 +79,61 @@ func TestLoadRejectsInvalidCooldownProfiles(t *testing.T) { }) } } + +// TestWarnsOnInvalidTopLevelCooldown checks that a top-level duration the +// cooldown library cannot parse, and would read as 0, is reported. It warns +// rather than failing Validate so that configs which start today still start. +func TestWarnsOnInvalidTopLevelCooldown(t *testing.T) { + for _, tc := range []struct { + name string + set func(*CooldownConfig) + want string + }{ + {"default weeks", func(c *CooldownConfig) { c.Default = "1w" }, "cooldown: invalid default"}, + {"default words", func(c *CooldownConfig) { c.Default = "7 days" }, "cooldown: invalid default"}, + {"default negative", func(c *CooldownConfig) { c.Default = "-3d" }, "cooldown: invalid default"}, + {"ecosystem", func(c *CooldownConfig) { c.Ecosystems = map[string]string{"npm": "3days"} }, "cooldown: invalid ecosystems.npm"}, + {"package", func(c *CooldownConfig) { c.Packages = map[string]string{"pkg:npm/lodash": "soon"} }, "cooldown: invalid packages.pkg:npm/lodash"}, + {"pattern", func(c *CooldownConfig) { c.PackagePatterns = map[string]string{"pkg:npm/*": "1w"} }, "cooldown: invalid package_patterns.pkg:npm/*"}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := Default() + tc.set(&cfg.Cooldown) + if err := cfg.Validate(); err != nil { + t.Fatalf("Validate = %v, want nil: invalid top-level durations warn", err) + } + if !hasWarning(cfg.Warnings(), tc.want) { + t.Fatalf("Warnings = %q, want one containing %q", cfg.Warnings(), tc.want) + } + }) + } +} + +func TestNoWarningForValidTopLevelCooldown(t *testing.T) { + for _, value := range []string{"", "0", "3d", "1.5d", "48h", "90m"} { + cfg := Default() + cfg.Cooldown.Default = value + cfg.Cooldown.Ecosystems = map[string]string{"npm": value} + if hasWarning(cfg.Warnings(), "cooldown") { + t.Errorf("Warnings(%q) = %q, want no cooldown warning", value, cfg.Warnings()) + } + } +} + +func TestWarnsOnInvalidCooldownFromEnv(t *testing.T) { + t.Setenv("PROXY_COOLDOWN_DEFAULT", "1w") + cfg := Default() + cfg.LoadFromEnv() + if !hasWarning(cfg.Warnings(), "cooldown: invalid default") { + t.Fatalf("Warnings = %q, want invalid cooldown default", cfg.Warnings()) + } +} + +func hasWarning(warnings []string, substr string) bool { + for _, w := range warnings { + if strings.Contains(w, substr) { + return true + } + } + return false +} diff --git a/internal/config/mirror.go b/internal/config/mirror.go index 3b56fd87..cd0bdd2f 100644 --- a/internal/config/mirror.go +++ b/internal/config/mirror.go @@ -144,6 +144,9 @@ func (c *Config) Warnings() []string { if c.Mirror.Refresh != nil && !c.CacheMetadata { warnings = append(warnings, "mirror.refresh without cache_metadata does not refresh Debian, Alpine or RPM packages: their indexes are found through the metadata cache") } + if w := c.Cooldown.durationWarning(); w != "" { + warnings = append(warnings, w) + } return warnings } From 4c3cdfecbde414041e1286c4b951c43507b9bb07 Mon Sep 17 00:00:00 2001 From: bc Date: Sun, 15 Nov 2026 07:08:30 +0100 Subject: [PATCH 2/3] Leave invalid cooldown pattern durations to the policy - Warn only for default, ecosystems and packages; the cooldown library already rejects an invalid package_patterns duration when the policy is built, so startup fails before warnings are logged - Correct the documentation to say pattern durations are rejected - Test that an invalid pattern fails the policy and does not warn --- docs/configuration.md | 2 +- internal/config/cooldown_profiles.go | 14 ++++++++------ internal/config/cooldown_profiles_test.go | 15 ++++++++++++++- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 4ac2806b..bf20c275 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1275,7 +1275,7 @@ cooldown: | `cooldown.packages` | - | Per-package overrides (keyed by PURL) | | `cooldown.package_patterns` | - | Per-package glob overrides (keyed by PURL glob) | -Durations support days (`7d`), hours (`48h`), and minutes (`30m`). Set to `0` to disable. Other units such as weeks (`1w`) are not supported: a top-level value that can't be parsed applies no cooldown, so the proxy logs a warning at startup naming it. Invalid durations inside `cooldown.profiles` are rejected at startup. +Durations support days (`7d`), hours (`48h`), and minutes (`30m`). Set to `0` to disable. Other units such as weeks (`1w`) are not supported. A `default`, `ecosystems` or `packages` value that can't be parsed applies no cooldown, so the proxy logs a warning at startup naming it. Invalid durations in `package_patterns` and inside `cooldown.profiles` are rejected at startup. Package PURL keys are normalized to canonical form before matching, so `pkg:npm/@babel/core` and `pkg:npm/%40babel/core` are equivalent, as are `pkg:pypi/Django` and `pkg:pypi/django`. If both forms configure the same package, the canonical entry wins. diff --git a/internal/config/cooldown_profiles.go b/internal/config/cooldown_profiles.go index 95752621..c502a2d6 100644 --- a/internal/config/cooldown_profiles.go +++ b/internal/config/cooldown_profiles.go @@ -18,14 +18,16 @@ func (c *CooldownConfig) Policy() (*cooldown.Policy, error) { }, c.PackagePatterns) } -// durationWarning describes a top-level cooldown duration that cannot be -// parsed, or returns "" when they all parse. +// durationWarning describes a top-level default, ecosystem or package +// cooldown duration that cannot be parsed, or returns "" when they all parse. // -// The cooldown library reads a duration it cannot parse as 0, so a typo such -// as "1w" turns cooldown off without notice. Profiles already reject such -// values; the top level only warns, so configs that start today keep starting. +// The cooldown library reads such a duration as 0, so a typo such as "1w" +// turns cooldown off without notice. Profiles already reject these values, as +// does the library for package_patterns; the top level only warns, so configs +// that start today keep starting. func (c *CooldownConfig) durationWarning() string { - if err := c.validateProfileDurations(); err != nil { + top := CooldownConfig{Default: c.Default, Ecosystems: c.Ecosystems, Packages: c.Packages} + if err := top.validateProfileDurations(); err != nil { return fmt.Sprintf("cooldown: %v; no cooldown applies where this value is used", err) } return "" diff --git a/internal/config/cooldown_profiles_test.go b/internal/config/cooldown_profiles_test.go index 7b9a9893..26591f9a 100644 --- a/internal/config/cooldown_profiles_test.go +++ b/internal/config/cooldown_profiles_test.go @@ -94,7 +94,6 @@ func TestWarnsOnInvalidTopLevelCooldown(t *testing.T) { {"default negative", func(c *CooldownConfig) { c.Default = "-3d" }, "cooldown: invalid default"}, {"ecosystem", func(c *CooldownConfig) { c.Ecosystems = map[string]string{"npm": "3days"} }, "cooldown: invalid ecosystems.npm"}, {"package", func(c *CooldownConfig) { c.Packages = map[string]string{"pkg:npm/lodash": "soon"} }, "cooldown: invalid packages.pkg:npm/lodash"}, - {"pattern", func(c *CooldownConfig) { c.PackagePatterns = map[string]string{"pkg:npm/*": "1w"} }, "cooldown: invalid package_patterns.pkg:npm/*"}, } { t.Run(tc.name, func(t *testing.T) { cfg := Default() @@ -137,3 +136,17 @@ func hasWarning(warnings []string, substr string) bool { } return false } + +// TestInvalidCooldownPatternFailsPolicy checks that an invalid package_patterns +// duration is left to the cooldown library, which already refuses it when the +// policy is built at startup, rather than reported as a warning. +func TestInvalidCooldownPatternFailsPolicy(t *testing.T) { + cfg := Default() + cfg.Cooldown.PackagePatterns = map[string]string{"pkg:npm/*": "1w"} + if _, err := cfg.Cooldown.Policy(); err == nil { + t.Fatal("Policy() = nil error, want the invalid pattern duration rejected") + } + if hasWarning(cfg.Warnings(), "cooldown") { + t.Errorf("Warnings = %q, want no cooldown warning for a pattern", cfg.Warnings()) + } +} From b00e2572307e35c9948ac9b558203d1f8f21f6d1 Mon Sep 17 00:00:00 2001 From: bc Date: Sun, 15 Nov 2026 16:58:44 +0100 Subject: [PATCH 3/3] List every invalid cooldown duration in one warning - Report all unparsable default, ecosystems and packages values at once, so fixing a config takes one restart - Keep profiles failing on the first invalid duration - Add a test with several invalid values --- internal/config/cooldown_profiles.go | 34 ++++++++++++++++------- internal/config/cooldown_profiles_test.go | 15 ++++++++++ 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/internal/config/cooldown_profiles.go b/internal/config/cooldown_profiles.go index c502a2d6..44391a06 100644 --- a/internal/config/cooldown_profiles.go +++ b/internal/config/cooldown_profiles.go @@ -5,6 +5,7 @@ import ( "maps" "regexp" "slices" + "strings" "github.com/git-pkgs/cooldown" ) @@ -18,8 +19,8 @@ func (c *CooldownConfig) Policy() (*cooldown.Policy, error) { }, c.PackagePatterns) } -// durationWarning describes a top-level default, ecosystem or package -// cooldown duration that cannot be parsed, or returns "" when they all parse. +// durationWarning lists every top-level default, ecosystem or package cooldown +// duration that cannot be parsed, or returns "" when they all parse. // // The cooldown library reads such a duration as 0, so a typo such as "1w" // turns cooldown off without notice. Profiles already reject these values, as @@ -27,10 +28,15 @@ func (c *CooldownConfig) Policy() (*cooldown.Policy, error) { // that start today keep starting. func (c *CooldownConfig) durationWarning() string { top := CooldownConfig{Default: c.Default, Ecosystems: c.Ecosystems, Packages: c.Packages} - if err := top.validateProfileDurations(); err != nil { - return fmt.Sprintf("cooldown: %v; no cooldown applies where this value is used", err) + errs := top.durationErrors() + if len(errs) == 0 { + return "" } - return "" + msgs := make([]string, len(errs)) + for i, err := range errs { + msgs[i] = err.Error() + } + return fmt.Sprintf("cooldown: %s; no cooldown applies where these values are used", strings.Join(msgs, "; ")) } // ValidateProfiles checks names and policies before they become routes. @@ -54,6 +60,15 @@ func (c *CooldownConfig) ValidateProfiles() error { } func (c *CooldownConfig) validateProfileDurations() error { + if errs := c.durationErrors(); len(errs) > 0 { + return errs[0] + } + return nil +} + +// durationErrors returns one error per invalid duration, sorted by key. +func (c *CooldownConfig) durationErrors() []error { + var errs []error values := map[string]string{"default": c.Default} for group, overrides := range map[string]map[string]string{ "ecosystems": c.Ecosystems, "packages": c.Packages, "package_patterns": c.PackagePatterns, @@ -69,11 +84,10 @@ func (c *CooldownConfig) validateProfileDurations() error { } duration, err := cooldown.ParseDuration(value) if err != nil { - return fmt.Errorf("invalid %s: %w", key, err) - } - if duration < 0 { - return fmt.Errorf("invalid %s: duration must be non-negative", key) + errs = append(errs, fmt.Errorf("invalid %s: %w", key, err)) + } else if duration < 0 { + errs = append(errs, fmt.Errorf("invalid %s: duration must be non-negative", key)) } } - return nil + return errs } diff --git a/internal/config/cooldown_profiles_test.go b/internal/config/cooldown_profiles_test.go index 26591f9a..3be4666a 100644 --- a/internal/config/cooldown_profiles_test.go +++ b/internal/config/cooldown_profiles_test.go @@ -108,6 +108,21 @@ func TestWarnsOnInvalidTopLevelCooldown(t *testing.T) { } } +func TestWarnsOnEveryInvalidTopLevelCooldown(t *testing.T) { + cfg := Default() + cfg.Cooldown.Default = "1w" + cfg.Cooldown.Ecosystems = map[string]string{"npm": "3days", "pypi": "2d"} + cfg.Cooldown.Packages = map[string]string{"pkg:npm/lodash": "soon"} + for _, want := range []string{"invalid default", "invalid ecosystems.npm", "invalid packages.pkg:npm/lodash"} { + if !hasWarning(cfg.Warnings(), want) { + t.Errorf("Warnings = %q, want one containing %q", cfg.Warnings(), want) + } + } + if hasWarning(cfg.Warnings(), "ecosystems.pypi") { + t.Errorf("Warnings = %q, want the valid pypi value left out", cfg.Warnings()) + } +} + func TestNoWarningForValidTopLevelCooldown(t *testing.T) { for _, value := range []string{"", "0", "3d", "1.5d", "48h", "90m"} { cfg := Default()