sneak/webhooker
1
0
Fork 0
Code Issues 31 Pull Requests 5 Actions Packages Projects Releases Wiki Activity

Require a positive RETENTION_SWEEP_INTERVAL (closes #140) #142

Merged
clawbot merged 1 commits from issue-140-retention-sweep-interval-bound into next 2026-08-12 12:46:40 +02:00
Conversation 1 Commits 1 Files Changed 3 +81 -5
clawbot commented 2026-08-12 12:38:20 +02:00
Collaborator
Copy Link

Closes #140.

What changed

envDuration accepted 0s and negative values, and
RETENTION_SWEEP_INTERVAL feeds time.NewTicker in both the retention
reaper (internal/database/retention.go:122) and the archive sweeper
(internal/delivery/archive_sweeper.go:130). 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".

  • New envPositiveDuration in internal/config/config.go, structured
    exactly like envPort wrapping envPositiveInt: it delegates to
    envDuration and then rejects <= 0 with an error that wraps
    ErrNonPositiveValue and names the variable. Same sentinel and same
    failure shape as PORT and RECEIVER_RATE_LIMIT.
  • loadFromEnv uses it for RETENTION_SWEEP_INTERVAL.
  • Config tests: 0s and -1h cases added to TestRetentionSweepInterval,
    asserting via the existing expectStartupErrorFor helper that startup
    fails, that the message names RETENTION_SWEEP_INTERVAL, and that the
    error wraps config.ErrNonPositiveValue.
  • Regression cases added to TestSessionIdleTimeout asserting 0s and
    -1h still start successfully, so a future change cannot quietly turn
    the documented "disabled" case into a startup failure.
  • README: RETENTION_SWEEP_INTERVAL row added to the env table (it was
    missing), and the "Invalid values abort startup" section now states the
    positive requirement and calls out SESSION_IDLE_TIMEOUT as the
    deliberate exception.

envDuration audit

There are exactly two envDuration call sites in the tree:

Variable Reaches a ticker/timer? Result
RETENTION_SWEEP_INTERVAL yes — time.NewTicker in internal/database/retention.go:122 and internal/delivery/archive_sweeper.go:130 fixed here
SESSION_IDLE_TIMEOUT no left as-is, deliberately

SESSION_IDLE_TIMEOUT only ever becomes session.idleTimeout, which is
guarded by an explicit if s.idleTimeout <= 0 { return } at both of its
use sites (internal/session/session.go:264 and :390). Non-positive
there means idle expiry is disabled, which is documented in the README,
so it stays on envDuration.

The remaining NewTicker/AfterFunc sites take no
environment-configurable duration: internal/delivery/engine.go:619 uses
the retrySweepInterval constant, internal/delivery/engine.go:222 uses
a computed backoff delay, and the context.WithTimeout calls in
internal/delivery/ssrf.go:146 and internal/server/server.go:166 use
constants (and a non-positive timeout there would not panic in any case).

Gate evidence

make check — exit 0, tests executed (not cached):

--- PASS: TestRetentionSweepInterval/zero_fails_startup (0.02s)
--- PASS: TestRetentionSweepInterval/negative_fails_startup (0.00s)
--- PASS: TestSessionIdleTimeout/zero_disables_idle_expiry (0.00s)
--- PASS: TestSessionIdleTimeout/negative_disables_idle_expiry (0.01s)
0 issues.

docker build --no-cache-filter=lint,builder --progress=plain . — exit 0,
with both stages confirmed executed rather than CACHED:

#15 [lint 7/8] RUN make fmt-check
#16 [lint 8/8] RUN make lint
#16 61.04 0 issues.
#23 [builder  8/10] RUN make test
#23 65.86     --- PASS: TestRetentionSweepInterval/zero_fails_startup (0.00s)
#23 65.86     --- PASS: TestRetentionSweepInterval/negative_fails_startup (0.00s)

No parallel golangci-lint is running in either run. TODO.md untouched.

Closes https://git.eeqj.de/sneak/webhooker/issues/140. ## What changed `envDuration` accepted `0s` and negative values, and `RETENTION_SWEEP_INTERVAL` feeds `time.NewTicker` in both the retention reaper (`internal/database/retention.go:122`) and the archive sweeper (`internal/delivery/archive_sweeper.go:130`). `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". - New `envPositiveDuration` in `internal/config/config.go`, structured exactly like `envPort` wrapping `envPositiveInt`: it delegates to `envDuration` and then rejects `<= 0` with an error that wraps `ErrNonPositiveValue` and names the variable. Same sentinel and same failure shape as `PORT` and `RECEIVER_RATE_LIMIT`. - `loadFromEnv` uses it for `RETENTION_SWEEP_INTERVAL`. - Config tests: `0s` and `-1h` cases added to `TestRetentionSweepInterval`, asserting via the existing `expectStartupErrorFor` helper that startup fails, that the message names `RETENTION_SWEEP_INTERVAL`, and that the error wraps `config.ErrNonPositiveValue`. - Regression cases added to `TestSessionIdleTimeout` asserting `0s` and `-1h` still start successfully, so a future change cannot quietly turn the documented "disabled" case into a startup failure. - README: `RETENTION_SWEEP_INTERVAL` row added to the env table (it was missing), and the "Invalid values abort startup" section now states the positive requirement and calls out `SESSION_IDLE_TIMEOUT` as the deliberate exception. ## `envDuration` audit There are exactly two `envDuration` call sites in the tree: | Variable | Reaches a ticker/timer? | Result | | --- | --- | --- | | `RETENTION_SWEEP_INTERVAL` | yes — `time.NewTicker` in `internal/database/retention.go:122` and `internal/delivery/archive_sweeper.go:130` | fixed here | | `SESSION_IDLE_TIMEOUT` | no | left as-is, deliberately | `SESSION_IDLE_TIMEOUT` only ever becomes `session.idleTimeout`, which is guarded by an explicit `if s.idleTimeout <= 0 { return }` at both of its use sites (`internal/session/session.go:264` and `:390`). Non-positive there means idle expiry is disabled, which is documented in the README, so it stays on `envDuration`. The remaining `NewTicker`/`AfterFunc` sites take no environment-configurable duration: `internal/delivery/engine.go:619` uses the `retrySweepInterval` constant, `internal/delivery/engine.go:222` uses a computed backoff delay, and the `context.WithTimeout` calls in `internal/delivery/ssrf.go:146` and `internal/server/server.go:166` use constants (and a non-positive timeout there would not panic in any case). ## Gate evidence `make check` — exit 0, tests executed (not cached): ``` --- PASS: TestRetentionSweepInterval/zero_fails_startup (0.02s) --- PASS: TestRetentionSweepInterval/negative_fails_startup (0.00s) --- PASS: TestSessionIdleTimeout/zero_disables_idle_expiry (0.00s) --- PASS: TestSessionIdleTimeout/negative_disables_idle_expiry (0.01s) 0 issues. ``` `docker build --no-cache-filter=lint,builder --progress=plain .` — exit 0, with both stages confirmed executed rather than `CACHED`: ``` #15 [lint 7/8] RUN make fmt-check #16 [lint 8/8] RUN make lint #16 61.04 0 issues. #23 [builder 8/10] RUN make test #23 65.86 --- PASS: TestRetentionSweepInterval/zero_fails_startup (0.00s) #23 65.86 --- PASS: TestRetentionSweepInterval/negative_fails_startup (0.00s) ``` No `parallel golangci-lint is running` in either run. `TODO.md` untouched.
clawbot added the needs-review label 2026-08-12 12:38:20 +02:00
clawbot added 1 commit 2026-08-12 12:38:20 +02:00
Require a positive RETENTION_SWEEP_INTERVAL (closes #140)
All checks were successful
check / check (push) Successful in 2m59s
Details
500f39ac01 
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.
clawbot commented 2026-08-12 12:45:59 +02:00
Author
Collaborator
Copy Link

PASS. Independently verified: the envDuration audit (exactly two call sites; the only other NewTicker/AfterFunc/WithTimeout sites take constants or a computed backoff), that defaultRetentionSweepInterval also passes the new bound so the unset path cannot reach NewTicker with a non-positive period, that loadFromEnv is the sole production constructor of Config, and that both s.idleTimeout <= 0 guards (internal/session/session.go:264, :390) precede every use of idleTimeout. Mutation-checked: reverting to envDuration fails the two new subtests, and dropping the %w sentinel wrap alone also fails them, so neither assertion is vacuous.

Gate (run locally on 500f39a, not trusting the cached CI log — #119): docker build --no-cache-filter=lint,builder --progress=plain . exit 0, lint stage executed (RUN make lint 73.9s, 0 issues., no parallel golangci-lint), builder RUN make test executed with zero (cached) markers and the four new subtests observed PASS in-container.

Disclosure: script/lint in this repo runs the host golangci-lint, so the make check lint result is not container evidence; the Docker lint stage above is what I relied on.

PASS. Independently verified: the `envDuration` audit (exactly two call sites; the only other `NewTicker`/`AfterFunc`/`WithTimeout` sites take constants or a computed backoff), that `defaultRetentionSweepInterval` also passes the new bound so the unset path cannot reach `NewTicker` with a non-positive period, that `loadFromEnv` is the sole production constructor of `Config`, and that both `s.idleTimeout <= 0` guards (`internal/session/session.go:264`, `:390`) precede every use of `idleTimeout`. Mutation-checked: reverting to `envDuration` fails the two new subtests, and dropping the `%w` sentinel wrap alone also fails them, so neither assertion is vacuous. Gate (run locally on `500f39a`, not trusting the cached CI log — https://git.eeqj.de/sneak/webhooker/issues/119): `docker build --no-cache-filter=lint,builder --progress=plain .` exit 0, `lint` stage executed (`RUN make lint` 73.9s, `0 issues.`, no `parallel golangci-lint`), `builder` `RUN make test` executed with zero `(cached)` markers and the four new subtests observed PASS in-container. Disclosure: `script/lint` in this repo runs the host `golangci-lint`, so the `make check` lint result is not container evidence; the Docker `lint` stage above is what I relied on.
clawbot merged commit 3941f0b0ff into next 2026-08-12 12:46:40 +02:00
clawbot deleted branch issue-140-retention-sweep-interval-bound 2026-08-12 12:46:41 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-review
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#142
Delete Branch "issue-140-retention-sweep-interval-bound"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?