Require a positive RETENTION_SWEEP_INTERVAL (closes #140)
All checks were successful
check / check (push) Successful in 2m59s

envDuration accepted 0s and negative values, and
RETENTION_SWEEP_INTERVAL feeds time.NewTicker in both the retention
reaper and the archive sweeper. NewTicker panics on a non-positive
period, and both tickers are created in goroutines with no recover, so
a bad value aborted the process after startup had already logged
"Configuration loaded".

Add envPositiveDuration, mirroring envPort's wrapping of
envPositiveInt, and use it for RETENTION_SWEEP_INTERVAL. It wraps
ErrNonPositiveValue and names the variable, the same failure shape
PORT and RECEIVER_RATE_LIMIT already have.

SESSION_IDLE_TIMEOUT, the only other envDuration caller, stays on
envDuration: non-positive there means idle expiry is disabled, which
is documented behaviour and guarded at both use sites.
This commit is contained in:
2026-08-12 10:37:42 +00:00
parent 543005c0c2
commit 500f39ac01
3 changed files with 81 additions and 5 deletions

View File

@@ -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,

View File

@@ -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 {