From 3941f0b0ff2222e5e137519649b6a85e1d563eb5 Mon Sep 17 00:00:00 2001 From: clawbot Date: Wed, 12 Aug 2026 12:46:39 +0200 Subject: [PATCH] Require a positive RETENTION_SWEEP_INTERVAL (closes #140) A non-positive value reached time.NewTicker in the retention reaper and the archive sweeper, panicking both goroutines after startup had already reported success. envPositiveDuration now rejects it in loadFromEnv, matching how PORT and RECEIVER_RATE_LIMIT fail. SESSION_IDLE_TIMEOUT keeps treating non-positive as disabled, which is guarded at every use site. --- README.md | 9 +++++-- internal/config/config.go | 33 ++++++++++++++++++++++++- internal/config/config_test.go | 44 ++++++++++++++++++++++++++++++++-- 3 files changed, 81 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index d089ea3..12e1338 100644 --- a/README.md +++ b/README.md @@ -93,6 +93,7 @@ TTY detection, and security headers are always applied. | `METRICS_USERNAME` | Basic auth username for `/metrics` | `""` | | `METRICS_PASSWORD` | Basic auth password for `/metrics` | `""` | | `SENTRY_DSN` | Sentry error reporting DSN | `""` | +| `RETENTION_SWEEP_INTERVAL` | Retention reaper period (Go duration, must be positive) | `1h` | | `SESSION_IDLE_TIMEOUT` | Idle session timeout (Go duration) | `24h` | | `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint | `120` | | `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted | `""` (none) | @@ -173,8 +174,12 @@ its value and refuses to start, rather than silently running with a substituted default. `PORT=eighty`, `DEBUG=ture`, and `RETENTION_SWEEP_INTERVAL=1 hour` all abort startup. `PORT` must additionally be a number in the range 1–65535, -`RECEIVER_RATE_LIMIT` must be at least 1, and every entry in -`TRUSTED_PROXIES` must be a CIDR block or a bare IP address. +`RECEIVER_RATE_LIMIT` must be at least 1, +`RETENTION_SWEEP_INTERVAL` must be greater than zero (it is a ticker +period, so `0s` or a negative value would crash the reaper after +startup), and every entry in `TRUSTED_PROXIES` must be a CIDR block or +a bare IP address. `SESSION_IDLE_TIMEOUT` is the exception: a +non-positive value there means idle expiry is disabled, not invalid. Boolean variables (`DEBUG`, `MAINTENANCE_MODE`) accept exactly the spellings Go's `strconv.ParseBool` accepts — `1`, `t`, `T`, `TRUE`, diff --git a/internal/config/config.go b/internal/config/config.go index 27e875f..cae3820 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -92,6 +92,7 @@ type Config struct { SentryDSN string // RetentionSweepInterval is how often the retention reaper runs. + // Always positive: it becomes a time.NewTicker period. RetentionSweepInterval time.Duration // SessionIdleTimeout is the sliding inactivity window after @@ -235,6 +236,34 @@ func envDuration( return d, nil } +// envPositiveDuration returns the value of the named environment +// variable parsed as a Go duration that must be greater than zero. +// Returns defaultValue if not set. A set value that is unparseable or +// non-positive is a hard error naming the key and the bad value. +// +// This is for durations that reach time.NewTicker, which panics on a +// non-positive period, in a goroutine started after startup has +// already reported success. It is deliberately not used for durations +// where non-positive means "disabled" (SESSION_IDLE_TIMEOUT). +func envPositiveDuration( + key string, + defaultValue time.Duration, +) (time.Duration, error) { + d, err := envDuration(key, defaultValue) + if err != nil { + return 0, err + } + + if d <= 0 { + return 0, fmt.Errorf( + "%w: %s must be greater than zero, got %s", + ErrNonPositiveValue, key, d, + ) + } + + return d, nil +} + // parseCIDR parses one trusted-proxy list entry, which may be a // CIDR block ("10.0.0.0/8") or a bare address ("10.0.0.1", treated // as a single-host block). @@ -346,7 +375,7 @@ func loadFromEnv() (*Config, error) { return nil, err } - retentionSweepInterval, err := envDuration( + retentionSweepInterval, err := envPositiveDuration( "RETENTION_SWEEP_INTERVAL", defaultRetentionSweepInterval, ) @@ -354,6 +383,8 @@ func loadFromEnv() (*Config, error) { return nil, err } + // Non-positive is "disabled" here, not invalid, so this stays on + // envDuration. sessionIdleTimeout, err := envDuration( "SESSION_IDLE_TIMEOUT", defaultSessionIdleTimeout, diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 816d5a2..f8ff7be 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -139,7 +139,11 @@ func TestRetentionSweepInterval(t *testing.T) { set bool value string expectError bool - expected time.Duration + // sentinel, when set, must be wrapped by the startup + // error; every error case must additionally name the + // variable in its message. + sentinel error + expected time.Duration }{ { name: caseUnsetUsesDefault, @@ -158,6 +162,24 @@ func TestRetentionSweepInterval(t *testing.T) { value: "not-a-duration", expectError: true, }, + { + // A non-positive period panics the ticker in the + // reaper and archive-sweeper goroutines, long after + // startup has reported success, so it has to fail + // here instead. + name: "zero fails startup", + set: true, + value: "0s", + expectError: true, + sentinel: config.ErrNonPositiveValue, + }, + { + name: "negative fails startup", + set: true, + value: "-1h", + expectError: true, + sentinel: config.ErrNonPositiveValue, + }, } for _, tt := range tests { @@ -175,7 +197,9 @@ func TestRetentionSweepInterval(t *testing.T) { } if tt.expectError { - expectStartupError(t) + expectStartupErrorFor( + t, "RETENTION_SWEEP_INTERVAL", tt.sentinel, + ) } else { testRetentionSweepIntervalSuccess(t, tt.expected) } @@ -281,6 +305,22 @@ func TestSessionIdleTimeout(t *testing.T) { value: "not-a-duration", expectError: true, }, + { + // Non-positive is "idle expiry disabled" for this + // variable, not a configuration error: unlike + // RETENTION_SWEEP_INTERVAL it never becomes a ticker + // period. + name: "zero disables idle expiry", + set: true, + value: "0s", + expected: 0, + }, + { + name: "negative disables idle expiry", + set: true, + value: "-1h", + expected: -time.Hour, + }, } for _, tt := range tests {