2 Commits

Author SHA1 Message Date
clawbot
9b3baec214 Correct release-blocking documentation inaccuracies (closes #141)
Some checks failed
check / check (push) Has been cancelled
Four defects found by the integration review, each of which would have
made the README or the release notes untrue at the moment of tagging.

RETENTION_SWEEP_INTERVAL was absent from the README env table while two
other passages referred to it as documented. Enumerated every variable
read by internal/config from the source (12 in total) rather than by
eye; that was the only one missing.

TODO.md omitted five of the units landed in this milestone (#64, #79,
#90, #113, #118), two of them credential-exposure fixes, which are
precisely the entries a reader of the release notes wants to find. The
list is now derived from git log origin/main..origin/next.

The README sold Replay in the present tense as a core capability while
no redelivery code exists anywhere in the tree, and the roadmap entry
for it had been dropped without it being implemented. Every mention of
replay or redelivery in the README is now either marked planned or
already under a Planned heading: the Rationale item, the Use Cases
bullet, the Event model's field description, and the delivery-semantics
passage on target-type edits, which told an operator that a terminally
failed delivery was recoverable by hand when nothing can recover it.
The comment on failUnretryableRetry made the same claim and is
corrected with it. The roadmap entry is back in TODO.md.

The production JS asset shipped a console.log on load. The rest of the
file and every other shipped asset were checked; that was the only one
(alpine.min.js is vendored and untouched).

No behavioural change: the only non-comment, non-documentation edit is
the deleted console.log.
2026-08-12 10:56:22 +00:00
3941f0b0ff Require a positive RETENTION_SWEEP_INTERVAL (closes #140)
All checks were successful
check / check (push) Successful in 6s
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.
2026-08-12 12:46:39 +02:00
4 changed files with 89 additions and 13 deletions

View File

@@ -93,7 +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` | How often the retention reaper and archive sweeper run (Go duration) | `1h` |
| `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (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) |
@@ -174,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 165535,
`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`,
@@ -500,7 +504,7 @@ A programmatic access credential for API authentication.
#### Event
A captured incoming webhook request. Stores the complete HTTP request
data for replay and auditing.
data for auditing and for the planned replay capability.
| Field | Type | Description |
| -------------- | ------ | ----------- |
@@ -782,9 +786,10 @@ unknown) one while one of its deliveries is still `retrying`, both
recovery paths above terminally mark that delivery `failed` and record a
`DeliveryResult` naming the current target type as the reason, logging it
at warn level. The delivery is not re-dispatched under the new type — the
operator never asked for that delivery — and the event itself remains
stored in the per-webhook event database, so it can be redelivered
manually.
operator never asked for that delivery — and while the event itself
remains stored in the per-webhook event database, there is no way to
redeliver it: manual redelivery is planned, not implemented (see
[TODO.md](TODO.md)).
### Circuit Breaker (HTTP Targets with Retries)

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 {

View File

@@ -785,9 +785,9 @@ func (e *Engine) sweepSingleRetry(
// status retrying themselves. Re-dispatching under the new type
// would be a delivery the operator never asked for, and leaving
// the row retrying strands it forever, so the delivery is
// failed with a recorded reason and can be redelivered
// manually. Logged at warn, not error: this is operator-caused
// state, not a system fault.
// failed with a recorded reason. The event stays stored, but
// nothing redelivers it today. Logged at warn, not error: this
// is operator-caused state, not a system fault.
func (e *Engine) failUnretryableRetry(
webhookDB *gorm.DB,
webhookID string,