diff --git a/README.md b/README.md index 1514b61..d0e5924 100644 --- a/README.md +++ b/README.md @@ -89,9 +89,49 @@ TTY detection, and security headers are always applied. | `PORT` | HTTP listen port | `8080` | | `DATA_DIR` | Directory for all SQLite databases | `/var/lib/webhooker` | | `DEBUG` | Enable debug logging | `false` | +| `MAINTENANCE_MODE` | Serve the maintenance page | `false` | | `METRICS_USERNAME` | Basic auth username for `/metrics` | `""` | | `METRICS_PASSWORD` | Basic auth password for `/metrics` | `""` | | `SENTRY_DSN` | Sentry error reporting DSN | `""` | +| `SESSION_IDLE_TIMEOUT` | Idle session timeout (Go duration) | `24h` | +| `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint | `120` | + +Sessions are bounded by two independent clocks, and end at whichever +one runs out first: + +- **Idle expiry** (`SESSION_IDLE_TIMEOUT`, default `24h`) is a sliding + window. Every authenticated request pushes it forward, so a session + in continuous use never hits it, while an abandoned one expires a day + after its last use. Set it to `0` to disable idle expiry entirely; + the absolute cap below still applies. A set-but-unparseable value + aborts startup rather than silently falling back to the default. +- **Absolute expiry** is a fixed 7 days from login. Activity does + **not** extend it: after a week, every session ends and the user + authenticates again. + +Only requests that authenticate with the session count as activity, so +an unauthenticated request carrying the cookie cannot keep a session +alive. The idle timestamp is rewritten at most once per tenth of the +idle window rather than on every request, which means a session may +expire up to 10% early relative to the user's true last request, but +never late. + +#### Invalid values abort startup + +The defaults above apply **only** to variables that are unset (or set +to an empty string). A variable that is set but cannot be parsed is a +fatal configuration error: webhooker logs the offending variable and +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, and +`RECEIVER_RATE_LIMIT` must be at least 1. + +Boolean variables (`DEBUG`, `MAINTENANCE_MODE`) accept exactly the +spellings Go's `strconv.ParseBool` accepts — `1`, `t`, `T`, `TRUE`, +`true`, `True`, `0`, `f`, `F`, `FALSE`, `false`, `False` — and nothing +else. `yes`, `on`, and `off` are rejected rather than quietly treated +as false. On first startup, webhooker automatically generates a cryptographically secure session encryption key and stores it in the database. This key @@ -307,13 +347,29 @@ event routing. | `user_id` | UUID | Foreign key → User | | `name` | string | Human-readable name | | `description` | string | Optional description | -| `retention_days` | integer | Days to retain events (default: 30) | +| `retention_days` | integer | Days to retain events (default: 30; 0 means retain forever) | **Relations:** Belongs to User. Has many Entrypoints. Has many Targets. The `retention_days` field controls how long event data is kept in the webhook's dedicated database before automatic cleanup. +Setting `retention_days` to `0` means "retain events forever". Because +the column carries a default of 30, a literal zero cannot survive an +insert, so a zero is rewritten on save to a sentinel of `365 * 1000` +days (`database.RetentionForeverDays`). The retention reaper recognises +that sentinel and skips the webhook entirely, and the web UI displays +such a webhook's retention as "forever" rather than as a day count. + +A *finite* retention is capped at `database.MaxFiniteRetentionDays` +(106751 days, about 292 years), and a larger one is rejected with a +400. The cap is not arbitrary: the reaper computes its cutoff as a +`time.Duration`, an int64 nanosecond count, and a longer period +overflows it. An overflowed cutoff lands in the future, where it +matches every row, so the sweep would delete every event the webhook +has instead of none. The reaper also clamps the value it is given, so a +row written by an older version cannot trigger that either. + #### Entrypoint A receiver URL where external services POST webhook events. Each @@ -509,7 +565,7 @@ This separation provides: DB; the event database file is hard-deleted (permanently removed). - **Per-webhook retention** — the `retention_days` field on each webhook controls automatic cleanup of old events in that webhook's database - only. + only, or disables cleanup entirely when set to `0` (retain forever). - **Performance** — each webhook's database has its own WAL, its own page cache, and its own lock, so concurrent event ingestion across webhooks won't contend. @@ -531,6 +587,36 @@ older than the expiry are pruned each time the archive is (re)opened. An archive write failure is never silent success: the delivery records a failed attempt with the error and is marked failed. +Because reopens only happen on writes, an archive belonging to a webhook +that has stopped receiving events would never be pruned. A background +**archive sweeper** closes that gap: on the same interval as the event +retention reaper (`RETENTION_SWEEP_INTERVAL`) it prunes every archive +whose database target declares a positive expiry, whether or not the +webhook is still receiving traffic. The sweep never creates an archive — +a webhook whose archive file does not yet exist is skipped, not +initialised — it takes the same per-webhook lock the write path uses, so +it can never interleave with a write, and it leaves the archive closed +afterwards so the move-the-file-away workflow keeps working. Archives +with no expiry, or the expiry `never`, are not touched by the sweep at +all. + +Note that a webhook has one archive file but may carry more than one +`database` target, each with its own `expiry`. The shortest expiry +configured on any of them therefore governs the whole archive, and the +sweep applies it whether or not the webhook is still receiving events. +Configure a single `database` target per webhook unless you intend that. + +Deleting a webhook releases its archive: the delivery engine's cached +archive writer is dropped and its file handle closed, so nothing lingers +after the webhook is gone. The archive **file itself is deliberately +left on disk**. Unlike the event database — per-webhook working storage +that is hard-deleted with the webhook — an archive is long-term storage +an operator may still want to keep or move away for offline retention, +and destroying it as a side effect of deleting a webhook would be +unrecoverable. Removing `archive-{webhookID}.db` is the operator's call. +Deleting a webhook's last `database` target releases the writer the same +way, and for the same reason leaves the file alone. + The **Slack target type** sends webhook events as formatted messages to any Slack-compatible incoming webhook URL (works with Slack, Mattermost, and other compatible services). Each message includes event metadata @@ -636,6 +722,18 @@ This means: durable fallback that ensures no retry is permanently lost, even under extreme backpressure. +**Changing a target's type does not migrate in-flight deliveries.** Only +`http` and `slack` targets own durable retries; `database` and `log` +targets are fire-and-forget and never produce a `retrying` delivery. If a +target's `type` is edited from a retrying type to a non-retrying (or +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. + ### Circuit Breaker (HTTP Targets with Retries) HTTP targets with `max_retries` > 0 are protected by a **per-target circuit breaker** that @@ -689,17 +787,24 @@ just delayed until the target is healthy again. ### Rate Limiting -Global rate limiting middleware (e.g., per-IP throttling applied at the -router level) **must not** apply to webhook receiver endpoints. Webhook -endpoints receive automated traffic from external services at -unpredictable rates, and blanket rate limits would cause legitimate -deliveries to be dropped. +Global blanket rate limiting middleware (e.g., a per-IP throttle shared +with the web UI) **must not** apply to webhook receiver endpoints. +Webhook endpoints receive automated traffic from external services at +unpredictable rates, and blanket limits shared with other routes would +cause legitimate deliveries to be dropped. -Instead, each webhook has its own individually configurable rate limit, -applied within the webhook handler itself. By default, no rate limit is -applied — webhook endpoints accept traffic as fast as it arrives. Rate -limits can be configured per-webhook when needed (e.g., to protect -against a misbehaving sender). +The receiver instead has its own dedicated abuse limit, scoped to the +`/webhook/{uuid}` route only and keyed per client IP per entrypoint: one +misbehaving sender is throttled without affecting other senders of the +same entrypoint or the same sender's other entrypoints. The limit is +`RECEIVER_RATE_LIMIT` requests per minute (default 120, generous for +legitimate webhook senders). Requests over the limit receive HTTP 429 +with a `Retry-After` header. A set-but-invalid `RECEIVER_RATE_LIMIT` +value aborts startup rather than silently falling back to the default. + +Finer-grained per-webhook rate limits (configured in the web UI and +enforced in the webhook handler) can layer on top of this env-level +abuse limit later; they are tracked as future work. ### API Endpoints @@ -867,9 +972,17 @@ Applied to all routes in this order: 8. **Sentry** — Error reporting to Sentry (if `SENTRY_DSN` is set; configured with `Repanic: true` so panics still reach Recoverer) -Additionally, form endpoints (`/pages`, `/sources`, `/source/*`) apply a -**MaxBodySize** middleware that limits POST/PUT/PATCH request bodies to -1 MB using `http.MaxBytesReader`, preventing oversized form submissions. +Additionally, form endpoints (`/pages`, `/user/*`, `/sources`, +`/source/*`) apply a **MaxBodySize** middleware that limits +POST/PUT/PATCH request bodies to 1 MB. It is registered ahead of the +CSRF middleware in every one of those route groups, because +gorilla/csrf parses the form; if the cap were installed after it, form +parsing would run under net/http's 10 MB default and the 1 MB limit +would never apply. A request that declares a `Content-Length` over the +limit is answered with `413 Request Entity Too Large` before any other +middleware or handler runs; a chunked request, or one that lies about +its length, is hard-capped by `http.MaxBytesReader` and fails +downstream at form-parse time. ### Authentication @@ -891,7 +1004,8 @@ Additionally, form endpoints (`/pages`, `/sources`, `/source/*`) apply a - Production security headers on all responses: HSTS, X-Content-Type-Options (`nosniff`), X-Frame-Options (`DENY`), Content-Security-Policy, Referrer-Policy, and Permissions-Policy -- Request body size limits (1 MB) on all form POST endpoints +- Request body size limits (1 MB) on all form POST endpoints, enforced + by middleware that runs before CSRF parses the form - **CSRF protection** via [gorilla/csrf](https://github.com/gorilla/csrf) on all state-changing forms (cookie-based double-submit tokens with HMAC authentication). Applied to `/pages`, `/sources`, `/source`, and diff --git a/TODO.md b/TODO.md index c869649..7900d62 100644 --- a/TODO.md +++ b/TODO.md @@ -10,24 +10,52 @@ # Status -pre-1.0. No git tags exist. main (afe88c6) is a working webhook proxy +pre-1.0. No git tags exist. main (4f5ecb1) is a working webhook proxy with auth, CSRF/SSRF protections, login rate limiting, Slack target, -policy compliance (#6), and pinned lint tooling (#55). Note: TODO.md was +event retention (#63), the database archiving target (#43), the admin +password change flow (#65), policy compliance (#6), pinned lint tooling +(#55), and fail-loud configuration parsing (#80). Note: TODO.md was deliberately deleted from this repo in f9a9569 (2026-03-01, #6); its content was folded into the README TODO section, which this draft reconstructs as of 2026-07-06. # Next Step -Implement automatic event retention cleanup based on retention_days: a -periodic maintenance job that deletes Events, Deliveries, and -DeliveryResults older than the parent webhook's retention_days from each -per-webhook event database. The field exists on the Webhook model and -the README promises the behavior, but nothing enforces it, so event -databases currently grow without bound. +Manual event redelivery from the web UI (replay is a core promised +capability in the README rationale). # Completed Steps +- 2026-08-11 Web UI cleanup: nav terminology unified on Webhooks, the + Profile settings placeholder removed, a progressive-enhancement copy + button for the entrypoint URL, and retention form copy that states the + actual policy (deletion by the reaper, 0 retains forever) (#57) +- 2026-08-09 Inactivity-based session timeout: sliding idle expiry + (`SESSION_IDLE_TIMEOUT`, default `24h`) refreshed on authenticated + requests, with the 7-day absolute cap kept as an independent + backstop that activity never extends (#66) +- 2026-08-09 Restart recovery and the 60s retry sweep terminally fail an + orphaned `retrying` delivery whose target type no longer supports + retries, recording a `DeliveryResult` with the reason instead of + leaving the delivery stuck forever (#82) +- 2026-08-09 Root the delivery engine's worker pool and the retention + reaper's sweep loop at `context.Background()` rather than the fx + `OnStart` hook context (#97), which carries fx's 15s start timeout and + killed both roughly fifteen seconds after boot: the proxy silently + stopped delivering webhooks entirely, and the reaper never ran a + single sweep under its default one-hour interval +- 2026-08-09 Archive writer lifecycle (#89): deleting a webhook (or its + last `database` target) evicts the cached archive writer and closes + its handle while deliberately leaving `archive-{webhookID}.db` on + disk, and a new `ArchiveSweeper` prunes idle archives on the existing + `RETENTION_SWEEP_INTERVAL` without ever creating an archive file +- 2026-08-09 Configuration parsing fails loudly on set-but-unparseable + environment values: `envInt` removed in favour of `envPositiveInt` + plus a `PORT` range check, `envBool` now parses with + `strconv.ParseBool`, and defaults apply only to unset variables (#80) +- 2026-08-07 Automatic event retention cleanup based on + `retention_days`, deleting expired events, deliveries, and delivery + results from each per-webhook event database (#63) - 2026-08-07 Update golangci-lint to v2.12.2 (Docker image digest in `Dockerfile`, release-archive sha256 pins in `script/bootstrap`), adopt the canonical `.golangci.yml` (v2 `linters.settings` layout so @@ -56,8 +84,6 @@ databases currently grow without bound. # Future Steps -- Manual event redelivery from the web UI (replay is a core promised - capability in the README rationale) - Delivery status and retry management UI - Per-webhook rate limiting in the receiver handler (per-webhook config plus handler enforcement; global limits must not apply to receiver @@ -71,7 +97,7 @@ databases currently grow without bound. - event redelivery endpoint - OpenAPI specification - Analytics dashboard: success rates, response times, volume -- Session expiration tuning and a remember-me option +- A remember-me option at login - Password change and reset flow - Later, nice to have - email delivery target type diff --git a/cmd/webhooker/main.go b/cmd/webhooker/main.go index 59fc655..793f44c 100644 --- a/cmd/webhooker/main.go +++ b/cmd/webhooker/main.go @@ -40,9 +40,15 @@ func main() { handlers.New, middleware.New, delivery.New, + delivery.NewArchiveSweeper, // Wire *delivery.Engine as delivery.Notifier so the // webhook handler can notify the engine of new deliveries. func(e *delivery.Engine) delivery.Notifier { return e }, + // Wire *delivery.Engine as delivery.WebhookEvictor so + // deleting a webhook releases its archive writer. + func(e *delivery.Engine) delivery.WebhookEvictor { + return e + }, server.New, ), fx.Invoke( @@ -50,6 +56,7 @@ func main() { *server.Server, *delivery.Engine, *database.RetentionReaper, + *delivery.ArchiveSweeper, ) { }, ), diff --git a/internal/config/config.go b/internal/config/config.go index 414a3bb..d2511b9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -7,7 +7,6 @@ import ( "log/slog" "os" "strconv" - "strings" "time" "go.uber.org/fx" @@ -31,12 +30,35 @@ const ( // defaultRetentionSweepInterval is how often the retention // reaper deletes events older than each webhook's RetentionDays. defaultRetentionSweepInterval = time.Hour + + // defaultSessionIdleTimeout is how long a session may go without + // authenticated activity before it expires. + defaultSessionIdleTimeout = 24 * time.Hour + + // defaultReceiverRateLimit is the default number of requests + // per minute each client IP may send to a single webhook + // receiver entrypoint. Generous for legitimate webhook + // senders while bounding abuse of the one unauthenticated, + // internet-exposed endpoint. + defaultReceiverRateLimit = 120 + + // maxPort is the highest valid TCP port number. The lower + // bound (at least 1) is enforced by envPositiveInt. + maxPort = 65535 ) // ErrInvalidEnvironment is returned when WEBHOOKER_ENVIRONMENT // contains an unrecognised value. var ErrInvalidEnvironment = errors.New("invalid environment") +// ErrNonPositiveValue is returned when an environment variable that +// requires a positive integer is set to zero or a negative number. +var ErrNonPositiveValue = errors.New("value must be positive") + +// ErrInvalidPort is returned when an environment variable holding a +// TCP port number is set above the valid port range. +var ErrInvalidPort = errors.New("invalid port") + //nolint:revive // ConfigParams is a standard fx naming convention. type ConfigParams struct { fx.In @@ -60,6 +82,14 @@ type Config struct { // RetentionSweepInterval is how often the retention reaper runs. RetentionSweepInterval time.Duration + // SessionIdleTimeout is the sliding inactivity window after + // which a session expires. Non-positive disables idle expiry. + SessionIdleTimeout time.Duration + + // ReceiverRateLimit is the number of requests per minute each + // client IP may send to a single webhook receiver entrypoint. + ReceiverRateLimit int + params *ConfigParams log *slog.Logger } @@ -81,27 +111,81 @@ func envString(key string) string { } // envBool returns the value of the named environment variable -// parsed as a boolean. Returns defaultValue if not set. -func envBool(key string, defaultValue bool) bool { - if v := os.Getenv(key); v != "" { - return strings.EqualFold(v, "true") || v == "1" +// parsed as a boolean. Returns defaultValue if not set. If the +// variable is set but cannot be parsed, it returns a wrapped error +// naming the key and the bad value, so startup fails loudly rather +// than silently falling back to the default. +// +// Parsing is strconv.ParseBool, which accepts 1, t, T, TRUE, true, +// True, 0, f, F, FALSE, false and False. Anything else — "yes", +// "on", or a typo like "ture" — is an error rather than a silent +// false. +func envBool(key string, defaultValue bool) (bool, error) { + v := os.Getenv(key) + if v == "" { + return defaultValue, nil } - return defaultValue + b, err := strconv.ParseBool(v) + if err != nil { + return false, fmt.Errorf( + "invalid boolean for %s: %q: %w", key, v, err, + ) + } + + return b, nil } -// envInt returns the value of the named environment variable -// parsed as an integer. Returns defaultValue if not set or -// unparseable. -func envInt(key string, defaultValue int) int { - if v := os.Getenv(key); v != "" { - i, err := strconv.Atoi(v) - if err == nil { - return i - } +// envPositiveInt returns the value of the named environment variable +// parsed as a positive integer. Returns defaultValue if not set. If +// the variable is set but cannot be parsed, or parses to less than +// one, it returns a wrapped error naming the key and the bad value, +// so startup fails loudly rather than silently falling back to the +// default. +func envPositiveInt( + key string, + defaultValue int, +) (int, error) { + v := os.Getenv(key) + if v == "" { + return defaultValue, nil } - return defaultValue + i, err := strconv.Atoi(v) + if err != nil { + return 0, fmt.Errorf( + "invalid integer for %s: %q: %w", key, v, err, + ) + } + + if i < 1 { + return 0, fmt.Errorf( + "%w: %s must be at least 1, got %q", + ErrNonPositiveValue, key, v, + ) + } + + return i, nil +} + +// envPort returns the value of the named environment variable parsed +// as a TCP port number. Returns defaultValue if not set. A set value +// that is unparseable, below 1, or above maxPort is a hard error +// naming the key and the bad value. +func envPort(key string, defaultValue int) (int, error) { + port, err := envPositiveInt(key, defaultValue) + if err != nil { + return 0, err + } + + if port > maxPort { + return 0, fmt.Errorf( + "%w: %s must be at most %d, got %d", + ErrInvalidPort, key, maxPort, port, + ) + } + + return port, nil } // envDuration returns the value of the named environment variable @@ -128,32 +212,52 @@ func envDuration( return d, nil } -// New creates a Config by reading environment variables. -// -//nolint:revive // lc parameter is required by fx even if unused. -func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { - log := params.Logger.Get() - - // Determine environment from WEBHOOKER_ENVIRONMENT env var, - // default to dev +// resolveEnvironment reads WEBHOOKER_ENVIRONMENT, defaulting to +// dev, and rejects unrecognised values. +func resolveEnvironment() (string, error) { environment := os.Getenv("WEBHOOKER_ENVIRONMENT") if environment == "" { environment = EnvironmentDev } - // Validate environment if environment != EnvironmentDev && environment != EnvironmentProd { - return nil, fmt.Errorf( + return "", fmt.Errorf( "%w: WEBHOOKER_ENVIRONMENT must be '%s' or '%s', got '%s'", ErrInvalidEnvironment, EnvironmentDev, EnvironmentProd, environment, ) } - // Parse the retention sweep interval; a set-but-unparseable value - // is a hard error so fx aborts startup rather than silently using - // the default. + return environment, nil +} + +// loadFromEnv builds a Config from the environment. Every value that +// needs parsing fails loudly when it is set but unparseable: the +// documented defaults apply only to variables that are unset (or +// empty), never as a substitute for a value the operator actually +// provided. +func loadFromEnv() (*Config, error) { + environment, err := resolveEnvironment() + if err != nil { + return nil, err + } + + port, err := envPort("PORT", defaultPort) + if err != nil { + return nil, err + } + + debug, err := envBool("DEBUG", false) + if err != nil { + return nil, err + } + + maintenanceMode, err := envBool("MAINTENANCE_MODE", false) + if err != nil { + return nil, err + } + retentionSweepInterval, err := envDuration( "RETENTION_SWEEP_INTERVAL", defaultRetentionSweepInterval, @@ -162,21 +266,54 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { return nil, err } - // Load configuration values from environment variables - s := &Config{ + sessionIdleTimeout, err := envDuration( + "SESSION_IDLE_TIMEOUT", + defaultSessionIdleTimeout, + ) + if err != nil { + return nil, err + } + + receiverRateLimit, err := envPositiveInt( + "RECEIVER_RATE_LIMIT", + defaultReceiverRateLimit, + ) + if err != nil { + return nil, err + } + + return &Config{ DataDir: envString("DATA_DIR"), - Debug: envBool("DEBUG", false), - MaintenanceMode: envBool("MAINTENANCE_MODE", false), + Debug: debug, + MaintenanceMode: maintenanceMode, Environment: environment, MetricsUsername: envString("METRICS_USERNAME"), MetricsPassword: envString("METRICS_PASSWORD"), - Port: envInt("PORT", defaultPort), + Port: port, SentryDSN: envString("SENTRY_DSN"), RetentionSweepInterval: retentionSweepInterval, - log: log, - params: ¶ms, + SessionIdleTimeout: sessionIdleTimeout, + ReceiverRateLimit: receiverRateLimit, + }, nil +} + +// New creates a Config by reading environment variables. +// +//nolint:revive // lc parameter is required by fx even if unused. +func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { + log := params.Logger.Get() + + // A set-but-unparseable value anywhere in the environment is a + // hard error, so fx aborts startup rather than running with a + // silently substituted default. + s, err := loadFromEnv() + if err != nil { + return nil, err } + s.log = log + s.params = ¶ms + // Set default DataDir. All SQLite databases (main application // DB and per-webhook event DBs) live here. The same default is // used regardless of environment; override with DATA_DIR if @@ -197,6 +334,7 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { "maintenanceMode", s.MaintenanceMode, "dataDir", s.DataDir, "retentionSweepInterval", s.RetentionSweepInterval.String(), + "receiverRateLimit", s.ReceiverRateLimit, "hasSentryDSN", s.SentryDSN != "", "hasMetricsAuth", s.MetricsUsername != "" && s.MetricsPassword != "", diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 7e3c2c8..31067db 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -14,6 +14,14 @@ import ( "sneak.berlin/go/webhooker/internal/logger" ) +// Shared subtest names for the env-parsing tables below, which all +// exercise the same three cases against different variables. +const ( + caseUnsetUsesDefault = "unset uses default" + caseValidValueParsed = "valid value is parsed" + caseUnparseableFails = "unparseable value fails startup" +) + func TestEnvironmentConfig(t *testing.T) { tests := []struct { name string @@ -130,18 +138,18 @@ func TestRetentionSweepInterval(t *testing.T) { expected time.Duration }{ { - name: "unset uses default", + name: caseUnsetUsesDefault, set: false, expected: time.Hour, }, { - name: "valid value is parsed", + name: caseValidValueParsed, set: true, value: "15m", expected: 15 * time.Minute, }, { - name: "unparseable value fails startup", + name: caseUnparseableFails, set: true, value: "not-a-duration", expectError: true, @@ -163,7 +171,7 @@ func TestRetentionSweepInterval(t *testing.T) { } if tt.expectError { - testRetentionSweepIntervalError(t) + expectStartupError(t) } else { testRetentionSweepIntervalSuccess(t, tt.expected) } @@ -171,7 +179,9 @@ func TestRetentionSweepInterval(t *testing.T) { } } -func testRetentionSweepIntervalError(t *testing.T) { +// expectStartupError asserts that fx refuses to build the app, +// which is what a set-but-invalid environment value must cause. +func expectStartupError(t *testing.T) { t.Helper() var cfg *config.Config @@ -215,6 +225,82 @@ func testRetentionSweepIntervalSuccess( assert.Equal(t, expected, cfg.RetentionSweepInterval) } +func TestSessionIdleTimeout(t *testing.T) { + tests := []struct { + name string + set bool + value string + expectError bool + expected time.Duration + }{ + { + name: caseUnsetUsesDefault, + set: false, + expected: 24 * time.Hour, + }, + { + name: caseValidValueParsed, + set: true, + value: "30m", + expected: 30 * time.Minute, + }, + { + name: caseUnparseableFails, + set: true, + value: "not-a-duration", + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") + + if tt.set { + t.Setenv("SESSION_IDLE_TIMEOUT", tt.value) + } else { + require.NoError(t, os.Unsetenv( + "SESSION_IDLE_TIMEOUT", + )) + } + + if tt.expectError { + expectStartupError(t) + } else { + testSessionIdleTimeoutSuccess(t, tt.expected) + } + }) + } +} + +func testSessionIdleTimeoutSuccess( + t *testing.T, + expected time.Duration, +) { + t.Helper() + + var cfg *config.Config + + app := fxtest.New( + t, + fx.Provide( + globals.New, + logger.New, + config.New, + ), + fx.Populate(&cfg), + ) + require.NoError(t, app.Err()) + + app.RequireStart() + + defer app.RequireStop() + + assert.Equal(t, expected, cfg.SessionIdleTimeout) +} + func TestDefaultDataDir(t *testing.T) { for _, env := range []string{"", "dev", "prod"} { name := env @@ -258,3 +344,91 @@ func TestDefaultDataDir(t *testing.T) { }) } } + +func TestReceiverRateLimit(t *testing.T) { + tests := []struct { + name string + set bool + value string + expectError bool + expected int + }{ + { + name: caseUnsetUsesDefault, + set: false, + expected: 120, + }, + { + name: caseValidValueParsed, + set: true, + value: "30", + expected: 30, + }, + { + name: caseUnparseableFails, + set: true, + value: "not-a-number", + expectError: true, + }, + { + name: "zero fails startup", + set: true, + value: "0", + expectError: true, + }, + { + name: "negative fails startup", + set: true, + value: "-5", + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") + + if tt.set { + t.Setenv("RECEIVER_RATE_LIMIT", tt.value) + } else { + require.NoError(t, os.Unsetenv( + "RECEIVER_RATE_LIMIT", + )) + } + + if tt.expectError { + expectStartupError(t) + } else { + testReceiverRateLimitSuccess(t, tt.expected) + } + }) + } +} + +func testReceiverRateLimitSuccess( + t *testing.T, + expected int, +) { + t.Helper() + + var cfg *config.Config + + app := fxtest.New( + t, + fx.Provide( + globals.New, + logger.New, + config.New, + ), + fx.Populate(&cfg), + ) + require.NoError(t, app.Err()) + + app.RequireStart() + + defer app.RequireStop() + + assert.Equal(t, expected, cfg.ReceiverRateLimit) +} diff --git a/internal/config/env_test.go b/internal/config/env_test.go new file mode 100644 index 0000000..a127abe --- /dev/null +++ b/internal/config/env_test.go @@ -0,0 +1,409 @@ +package config_test + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/fx" + "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/globals" + "sneak.berlin/go/webhooker/internal/logger" +) + +// testEnvKey is a throwaway variable name used only by the helper +// tables below, so they cannot disturb real configuration. +const testEnvKey = "WEBHOOKER_TEST_VALUE" + +// Real configuration variables exercised by the config.New tests. +const ( + envKeyPort = "PORT" + envKeyDebug = "DEBUG" + envKeyMaintenanceMode = "MAINTENANCE_MODE" +) + +// envBoolCase is one row of the envBool table. +type envBoolCase struct { + name string + set bool + value string + defaultValue bool + expectError bool + expected bool +} + +// envBoolCases is the envBool table, kept out of the test body so +// the test itself stays readable. +func envBoolCases() []envBoolCase { + return []envBoolCase{ + { + name: "unset uses default false", + defaultValue: false, + expected: false, + }, + { + name: "unset uses default true", + defaultValue: true, + expected: true, + }, + { + name: "empty uses default true", + set: true, + value: "", + defaultValue: true, + expected: true, + }, + { + name: "true is parsed", + set: true, + value: "true", + expected: true, + }, + { + name: "one is parsed", + set: true, + value: "1", + expected: true, + }, + { + name: "False is parsed", + set: true, + value: "False", + defaultValue: true, + expected: false, + }, + { + name: "zero is parsed", + set: true, + value: "0", + defaultValue: true, + expected: false, + }, + { + name: "yes is rejected", + set: true, + value: "yes", + expectError: true, + }, + { + name: "on is rejected", + set: true, + value: "on", + expectError: true, + }, + { + name: "typo is rejected", + set: true, + value: "ture", + expectError: true, + }, + } +} + +func TestEnvBool(t *testing.T) { + for _, tt := range envBoolCases() { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + if tt.set { + t.Setenv(testEnvKey, tt.value) + } else { + require.NoError(t, os.Unsetenv(testEnvKey)) + } + + got, err := config.EnvBoolForTest( + testEnvKey, tt.defaultValue, + ) + + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), testEnvKey) + assert.Contains(t, err.Error(), tt.value) + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.expected, got) + }) + } +} + +func TestEnvPositiveInt(t *testing.T) { + const defaultValue = 7 + + tests := []struct { + name string + set bool + value string + expectError bool + errIs error + expected int + }{ + { + name: "unset returns the default integer", + expected: defaultValue, + }, + { + name: "empty returns the default integer", + set: true, + value: "", + expected: defaultValue, + }, + { + name: "positive value is parsed", + set: true, + value: "42", + expected: 42, + }, + { + name: "unparseable value is rejected", + set: true, + value: "not-a-number", + expectError: true, + }, + { + name: "zero is rejected", + set: true, + value: "0", + expectError: true, + errIs: config.ErrNonPositiveValue, + }, + { + name: "negative is rejected", + set: true, + value: "-5", + expectError: true, + errIs: config.ErrNonPositiveValue, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + if tt.set { + t.Setenv(testEnvKey, tt.value) + } else { + require.NoError(t, os.Unsetenv(testEnvKey)) + } + + got, err := config.EnvPositiveIntForTest( + testEnvKey, defaultValue, + ) + + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), testEnvKey) + assert.Contains(t, err.Error(), tt.value) + + if tt.errIs != nil { + require.ErrorIs(t, err, tt.errIs) + } + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.expected, got) + }) + } +} + +func TestEnvPort(t *testing.T) { + const defaultValue = 8080 + + tests := []struct { + name string + set bool + value string + expectError bool + errIs error + expected int + }{ + { + name: "unset returns the default port", + expected: defaultValue, + }, + { + name: "valid port is parsed", + set: true, + value: "9000", + expected: 9000, + }, + { + name: "highest port is accepted", + set: true, + value: "65535", + expected: 65535, + }, + { + name: "unparseable value is rejected", + set: true, + value: "not-a-port", + expectError: true, + }, + { + name: "zero is rejected", + set: true, + value: "0", + expectError: true, + errIs: config.ErrNonPositiveValue, + }, + { + name: "above the port range is rejected", + set: true, + value: "65536", + expectError: true, + errIs: config.ErrInvalidPort, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + if tt.set { + t.Setenv(testEnvKey, tt.value) + } else { + require.NoError(t, os.Unsetenv(testEnvKey)) + } + + got, err := config.EnvPortForTest( + testEnvKey, defaultValue, + ) + + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), testEnvKey) + + if tt.errIs != nil { + require.ErrorIs(t, err, tt.errIs) + } + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.expected, got) + }) + } +} + +// buildConfig constructs a Config through fx exactly as the +// application does, returning the config and any construction error. +func buildConfig(t *testing.T) (*config.Config, error) { + t.Helper() + + var cfg *config.Config + + app := fx.New( + fx.NopLogger, + fx.Provide( + globals.New, + logger.New, + config.New, + ), + fx.Populate(&cfg), + ) + + return cfg, app.Err() +} + +func TestNewRejectsBadEnvValues(t *testing.T) { + tests := []struct { + name string + key string + value string + expectError bool + check func(t *testing.T, cfg *config.Config) + }{ + { + name: "valid PORT is used", + key: envKeyPort, + value: "9001", + check: func(t *testing.T, cfg *config.Config) { + t.Helper() + assert.Equal(t, 9001, cfg.Port) + }, + }, + { + name: "unparseable PORT aborts startup", + key: envKeyPort, + value: "eighty-eighty", + expectError: true, + }, + { + name: "out-of-range PORT aborts startup", + key: envKeyPort, + value: "70000", + expectError: true, + }, + { + name: "valid DEBUG is used", + key: envKeyDebug, + value: "true", + check: func(t *testing.T, cfg *config.Config) { + t.Helper() + assert.True(t, cfg.Debug) + }, + }, + { + name: "unparseable DEBUG aborts startup", + key: envKeyDebug, + value: "ture", + expectError: true, + }, + { + name: "unparseable MAINTENANCE_MODE aborts startup", + key: envKeyMaintenanceMode, + value: "sometimes", + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") + t.Setenv(tt.key, tt.value) + + cfg, err := buildConfig(t) + + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.key) + assert.Contains(t, err.Error(), tt.value) + + return + } + + require.NoError(t, err) + require.NotNil(t, cfg) + tt.check(t, cfg) + }) + } +} + +// TestNewUsesDefaultsWhenUnset proves the fail-loud behaviour did not +// break the legitimate unset case: absent variables still get their +// documented defaults. +func TestNewUsesDefaultsWhenUnset(t *testing.T) { + t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") + + for _, key := range []string{ + envKeyPort, envKeyDebug, envKeyMaintenanceMode, + } { + require.NoError(t, os.Unsetenv(key)) + } + + cfg, err := buildConfig(t) + require.NoError(t, err) + require.NotNil(t, cfg) + + assert.Equal(t, 8080, cfg.Port) + assert.False(t, cfg.Debug) + assert.False(t, cfg.MaintenanceMode) +} diff --git a/internal/config/export_test.go b/internal/config/export_test.go new file mode 100644 index 0000000..c7fac51 --- /dev/null +++ b/internal/config/export_test.go @@ -0,0 +1,20 @@ +package config + +// This file exposes the unexported environment parsing helpers to +// the external config_test package so each helper can be covered by +// its own table-driven test without weakening the package API. + +// EnvBoolForTest exposes envBool. +func EnvBoolForTest(key string, defaultValue bool) (bool, error) { + return envBool(key, defaultValue) +} + +// EnvPositiveIntForTest exposes envPositiveInt. +func EnvPositiveIntForTest(key string, defaultValue int) (int, error) { + return envPositiveInt(key, defaultValue) +} + +// EnvPortForTest exposes envPort. +func EnvPortForTest(key string, defaultValue int) (int, error) { + return envPort(key, defaultValue) +} diff --git a/internal/database/database_test.go b/internal/database/database_test.go index 22f7312..3b6a939 100644 --- a/internal/database/database_test.go +++ b/internal/database/database_test.go @@ -18,6 +18,11 @@ const ( testVersion = "test" // testContentType is the event content type used in tests. testContentType = "application/json" + // testWebhookName is the Webhook.Name used in tests. + testWebhookName = "test-webhook" + // testForeverLabel is Webhook.RetentionLabel for a retain-forever + // webhook. + testForeverLabel = "forever" ) func setupTestDB( diff --git a/internal/database/export_test.go b/internal/database/export_test.go index 29321fe..fdf8804 100644 --- a/internal/database/export_test.go +++ b/internal/database/export_test.go @@ -5,6 +5,8 @@ import ( "log/slog" "os" "time" + + "go.uber.org/fx" ) // NewTestRetentionReaper builds a RetentionReaper backed by the given @@ -29,3 +31,26 @@ func NewTestRetentionReaper( func (r *RetentionReaper) ExportSweep(ctx context.Context) { r.sweep(ctx) } + +// ExportRegisterHooks registers the reaper's real fx lifecycle hooks +// on a lifecycle supplied by a test, so a test can drive the exact +// OnStart/OnStop functions the application runs and hand OnStart the +// kind of context fx actually supplies. +func (r *RetentionReaper) ExportRegisterHooks(lc fx.Lifecycle) { + r.registerHooks(lc) +} + +// ExportStart starts the reaper's background loop for tests. +func (r *RetentionReaper) ExportStart() { + r.start() +} + +// ExportStop stops the reaper's background loop for tests. +func (r *RetentionReaper) ExportStop() { + r.stop() +} + +// ExportSetInterval overrides the sweep interval for tests. +func (r *RetentionReaper) ExportSetInterval(d time.Duration) { + r.interval = d +} diff --git a/internal/database/model_webhook.go b/internal/database/model_webhook.go index 9b47516..eedb192 100644 --- a/internal/database/model_webhook.go +++ b/internal/database/model_webhook.go @@ -1,6 +1,59 @@ package database +import ( + "math" + "strconv" + "time" + + "gorm.io/gorm" +) + +const ( + // DefaultRetentionDays is the event retention period applied to a + // webhook created without an explicit retention value. It is the + // single source of truth for that policy and must stay in sync + // with the `gorm:"default:30"` column default on + // Webhook.RetentionDays below; a struct tag cannot reference a + // constant, so a test asserts the two agree. + DefaultRetentionDays = 30 + + // RetentionForeverDays is the sentinel RetentionDays value meaning + // "retain events forever". Users express that intent as 0, which + // Webhook.BeforeSave rewrites to this value: the column default + // substitutes DefaultRetentionDays for a zero value at insert + // time, so a zero can never survive a round trip to the database. + // Nothing outside this file may hardcode the number. + RetentionForeverDays = 365 * 1000 + + // MaxFiniteRetentionDays is the largest finite retention period the + // reaper's cutoff arithmetic can represent, and therefore the + // largest one a caller may request. It is derived from that + // arithmetic rather than picked: retentionCutoff computes + // retentionDays * hoursPerDay * time.Hour, and a time.Duration is + // an int64 nanosecond count, so math.MaxInt64 nanoseconds divided + // by an hour and then by a day is the exact ceiling — 106751 days, + // a little over 292 years. + // + // One day more overflows int64, wraps the product negative, and + // turns the cutoff into a timestamp in the far future that matches + // every row in the webhook's database. That is why this bound is + // enforced on input and why retentionCutoff saturates underneath + // it. Note that RetentionForeverDays deliberately sits above this + // ceiling: such webhooks are skipped before any cutoff is + // computed, and never reach the arithmetic at all. + MaxFiniteRetentionDays = int( + math.MaxInt64 / int64(time.Hour) / hoursPerDay, + ) +) + // Webhook represents a webhook processing unit that groups entrypoints and targets +// +// Every method below takes a pointer receiver. BeforeSave has to, +// because it mutates the record and GORM only invokes hooks declared +// that way; the display helpers follow suit so the receiver kinds do +// not mix. Handlers therefore put a *Webhook into template data: +// html/template cannot call a pointer method on a value held in a map, +// because a map element is not addressable. type Webhook struct { BaseModel @@ -8,7 +61,9 @@ type Webhook struct { Name string `gorm:"not null" json:"name"` Description string `json:"description"` - // RetentionDays is the number of days to retain events. + // RetentionDays is the number of days to retain events. A value of + // RetentionForeverDays means retain forever. The column default + // must equal DefaultRetentionDays. RetentionDays int `gorm:"default:30" json:"retentionDays"` // Relations @@ -16,3 +71,55 @@ type Webhook struct { Entrypoints []Entrypoint `json:"entrypoints,omitempty"` Targets []Target `json:"targets,omitempty"` } + +// BeforeSave normalises RetentionDays on every insert and update. A +// non-positive value is the user's way of asking for "retain forever", +// which is stored as the RetentionForeverDays sentinel. +// +// This has to happen in a hook rather than at the call sites. GORM +// substitutes the column default (DefaultRetentionDays) for a zero +// value while building the insert statement, which runs after +// BeforeSave; rewriting any later than this loses that race and the +// row lands at 30 days. Living on the model also means a future call +// site — a REST API, a fixture, a migration — cannot bypass it. +func (w *Webhook) BeforeSave(_ *gorm.DB) error { + if w.RetentionDays <= 0 { + w.RetentionDays = RetentionForeverDays + } + + return nil +} + +// retainsForever reports whether a stored RetentionDays value means +// "keep events indefinitely". It is the single definition of that +// question, shared by Webhook.RetainsForever and by the reaper's +// cutoff computation so the two cannot disagree about which webhooks +// are exempt from reaping. +// +// It accepts the RetentionForeverDays sentinel written by BeforeSave +// and, defensively, the non-positive values that rows written before +// the sentinel existed may still carry. +func retainsForever(retentionDays int) bool { + return retentionDays <= 0 || + retentionDays >= RetentionForeverDays +} + +// RetainsForever reports whether this webhook's events are kept +// indefinitely. +func (w *Webhook) RetainsForever() bool { + return retainsForever(w.RetentionDays) +} + +// RetentionLabel returns the webhook's retention policy as display +// text, so that no template has to know about the sentinel value. +func (w *Webhook) RetentionLabel() string { + if w.RetainsForever() { + return "forever" + } + + if w.RetentionDays == 1 { + return "1 day" + } + + return strconv.Itoa(w.RetentionDays) + " days" +} diff --git a/internal/database/model_webhook_test.go b/internal/database/model_webhook_test.go new file mode 100644 index 0000000..d8be7fa --- /dev/null +++ b/internal/database/model_webhook_test.go @@ -0,0 +1,222 @@ +package database_test + +import ( + "context" + "reflect" + "strconv" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" + "gorm.io/gorm/clause" + "sneak.berlin/go/webhooker/internal/database" +) + +// startedTestDB returns a started main database for model-level tests. +func startedTestDB(t *testing.T) *gorm.DB { + t.Helper() + + db, lc := setupTestDB(t) + + ctx := context.Background() + require.NoError(t, lc.Start(ctx)) + t.Cleanup(func() { require.NoError(t, lc.Stop(ctx)) }) + + return db.DB() +} + +// storedRetention reads the retention_days column straight out of the +// row, so the assertion is about what was persisted rather than about +// whatever the in-memory struct happens to hold. +func storedRetention(t *testing.T, db *gorm.DB, id string) int { + t.Helper() + + var got int + + require.NoError( + t, + db.Model(&database.Webhook{}). + Where("id = ?", id). + Pluck("retention_days", &got).Error, + ) + + return got +} + +// newWebhookWithRetention creates a webhook through the ordinary Create +// path, so the BeforeSave hook and the GORM column default both apply +// exactly as they do in production. +func newWebhookWithRetention( + t *testing.T, + db *gorm.DB, + wh *database.Webhook, +) string { + t.Helper() + + wh.UserID = uuid.New().String() + wh.Name = testWebhookName + + require.NoError( + t, + db.Omit(clause.Associations).Create(wh).Error, + ) + + return wh.ID +} + +func TestWebhookBeforeSave_ZeroBecomesForeverSentinel(t *testing.T) { + t.Parallel() + + db := startedTestDB(t) + + wh := &database.Webhook{RetentionDays: 0} + id := newWebhookWithRetention(t, db, wh) + + assert.Equal( + t, + database.RetentionForeverDays, + storedRetention(t, db, id), + "a zero retention must be stored as the sentinel, "+ + "not replaced by the column default", + ) +} + +func TestWebhookBeforeSave_NegativeBecomesForeverSentinel(t *testing.T) { + t.Parallel() + + db := startedTestDB(t) + + wh := &database.Webhook{RetentionDays: -5} + id := newWebhookWithRetention(t, db, wh) + + assert.Equal( + t, + database.RetentionForeverDays, + storedRetention(t, db, id), + ) +} + +func TestWebhookBeforeSave_PositiveIsPreserved(t *testing.T) { + t.Parallel() + + db := startedTestDB(t) + + wh := &database.Webhook{RetentionDays: 7} + id := newWebhookWithRetention(t, db, wh) + + assert.Equal(t, 7, storedRetention(t, db, id)) +} + +// TestWebhookBeforeSave_UpdateToZeroBecomesSentinel proves the hook +// fires on update as well as insert, via the same Save call the edit +// handler makes. +func TestWebhookBeforeSave_UpdateToZeroBecomesSentinel(t *testing.T) { + t.Parallel() + + db := startedTestDB(t) + + wh := &database.Webhook{RetentionDays: 30} + id := newWebhookWithRetention(t, db, wh) + require.Equal(t, 30, storedRetention(t, db, id)) + + wh.RetentionDays = 0 + require.NoError(t, db.Omit(clause.Associations).Save(wh).Error) + + assert.Equal( + t, + database.RetentionForeverDays, + storedRetention(t, db, id), + ) +} + +// TestWebhookRetentionColumnDefaultMatchesConstant guards the one place +// the default lives twice: a struct tag cannot reference a constant, so +// this asserts the tag and DefaultRetentionDays agree. +func TestWebhookRetentionColumnDefaultMatchesConstant(t *testing.T) { + t.Parallel() + + field, ok := reflect.TypeFor[database.Webhook](). + FieldByName("RetentionDays") + require.True(t, ok, "Webhook.RetentionDays must exist") + + assert.Equal( + t, + "default:"+strconv.Itoa(database.DefaultRetentionDays), + field.Tag.Get("gorm"), + ) +} + +// TestMaxFiniteRetentionDaysIsTheOverflowCeiling asserts that the +// constant is exactly where the cutoff arithmetic stops working, which +// is what makes it a derived bound rather than a round number someone +// liked. One day more wraps the int64 nanosecond count negative, and a +// negative span is precisely what turned a cutoff into a future +// timestamp that matched — and deleted — every row. +// +// The multiplications are done through variables on purpose: as +// constant expressions the overflowing one would not compile. +func TestMaxFiniteRetentionDaysIsTheOverflowCeiling(t *testing.T) { + t.Parallel() + + const hoursPerDay = 24 + + atCeiling := database.MaxFiniteRetentionDays + overCeiling := database.MaxFiniteRetentionDays + 1 + + assert.Positive( + t, + time.Duration(atCeiling*hoursPerDay)*time.Hour, + "the ceiling itself must still be representable", + ) + assert.Negative( + t, + time.Duration(overCeiling*hoursPerDay)*time.Hour, + "one day past the ceiling must overflow", + ) + + assert.Less( + t, + database.MaxFiniteRetentionDays, + database.RetentionForeverDays, + "the sentinel sits above the ceiling and is only safe "+ + "because retain-forever webhooks skip the arithmetic", + ) +} + +func TestWebhookRetainsForeverAndLabel(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + days int + forever bool + label string + }{ + { + "sentinel", + database.RetentionForeverDays, true, testForeverLabel, + }, + { + "above sentinel", + database.RetentionForeverDays + 1, true, testForeverLabel, + }, + {"legacy zero", 0, true, testForeverLabel}, + {"legacy negative", -1, true, testForeverLabel}, + {"default", database.DefaultRetentionDays, false, "30 days"}, + {"one day", 1, false, "1 day"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + wh := database.Webhook{RetentionDays: tc.days} + + assert.Equal(t, tc.forever, wh.RetainsForever()) + assert.Equal(t, tc.label, wh.RetentionLabel()) + }) + } +} diff --git a/internal/database/retention.go b/internal/database/retention.go index 23d516f..dde29fc 100644 --- a/internal/database/retention.go +++ b/internal/database/retention.go @@ -56,9 +56,20 @@ func NewRetentionReaper( interval: params.Config.RetentionSweepInterval, } + r.registerHooks(lc) + + return r +} + +// registerHooks wires the reaper's start and stop into the fx +// lifecycle. The start hook's context is deliberately ignored: see +// start for why the sweep loop must not inherit it. +func (r *RetentionReaper) registerHooks(lc fx.Lifecycle) { lc.Append(fx.Hook{ - OnStart: func(ctx context.Context) error { - r.start(ctx) + //nolint:contextcheck // Not inheriting the hook context is + // the point: see start. + OnStart: func(_ context.Context) error { + r.start() return nil }, @@ -68,12 +79,20 @@ func NewRetentionReaper( return nil }, }) - - return r } -func (r *RetentionReaper) start(ctx context.Context) { - ctx, cancel := context.WithCancel(ctx) +// start launches the background sweep loop. +// +// The loop's context is derived from context.Background(), NOT from +// the fx OnStart hook context. The hook context carries fx's start +// timeout (15s by default) and is cancelled once the start phase +// completes, so a loop derived from it dies 45 minutes before its +// first tick under the default one-hour sweep interval, leaving a +// reaper that never reaps. A long-lived goroutine must outlive the +// startup phase, so its lifetime is bounded by OnStop instead: stop +// cancels this context and waits on the WaitGroup. +func (r *RetentionReaper) start() { + ctx, cancel := context.WithCancel(context.Background()) r.cancel = cancel r.wg.Add(1) @@ -114,7 +133,8 @@ func (r *RetentionReaper) run(ctx context.Context) { } // sweep lists every webhook from the main database and reaps expired -// rows from each per-webhook database whose RetentionDays is positive. +// rows from each per-webhook database that has a finite retention +// policy. Webhooks set to retain forever are skipped entirely. func (r *RetentionReaper) sweep(ctx context.Context) { var webhooks []Webhook @@ -139,8 +159,13 @@ func (r *RetentionReaper) sweep(ctx context.Context) { wh := webhooks[i] - // RetentionDays of zero or less means retain forever. - if wh.RetentionDays <= 0 { + // Skip retain-forever webhooks before building any query. + // RetainsForever covers both the RetentionForeverDays + // sentinel and the non-positive values that predate it: the + // sentinel is a positive number, so without this the reaper + // would compute a cutoff a thousand years in the past and + // issue a DELETE matching nothing on every single sweep. + if wh.RetainsForever() { continue } @@ -171,9 +196,10 @@ func (r *RetentionReaper) reapWebhook( return } - cutoff := time.Now().Add( - -time.Duration(retentionDays*hoursPerDay) * time.Hour, - ) + cutoff, ok := retentionCutoff(time.Now(), retentionDays) + if !ok { + return + } deleted, err := reapExpired(db, cutoff) if err != nil { @@ -196,6 +222,37 @@ func (r *RetentionReaper) reapWebhook( } } +// retentionCutoff returns the timestamp before which a webhook's +// events have expired, and whether any cutoff applies at all. It +// reports false for a retain-forever policy, so no DELETE is issued. +// +// The day count is clamped to MaxFiniteRetentionDays first. This is +// defense in depth rather than decoration: a time.Duration is an int64 +// nanosecond count, so an unclamped multiplication overflows above +// that ceiling and wraps the span negative. Subtracting a negative +// span moves the cutoff into the far future, where it matches every +// row in the database: the sweep then deletes every event, delivery, +// and delivery result, including ones created seconds ago. Rejecting +// out-of-range input at the form is the primary guard; saturating here +// means an old row, a migration, or a future call site cannot turn a +// too-large retention into total data loss. +func retentionCutoff( + now time.Time, + retentionDays int, +) (time.Time, bool) { + if retainsForever(retentionDays) { + return time.Time{}, false + } + + if retentionDays > MaxFiniteRetentionDays { + retentionDays = MaxFiniteRetentionDays + } + + return now.Add( + -time.Duration(retentionDays*hoursPerDay) * time.Hour, + ), true +} + // reapExpired hard-deletes, in foreign-key-safe order, the delivery // results, deliveries, and events associated with events older than // cutoff. Deletes are unscoped so rows are physically removed rather diff --git a/internal/database/retention_lifecycle_test.go b/internal/database/retention_lifecycle_test.go new file mode 100644 index 0000000..47e8d80 --- /dev/null +++ b/internal/database/retention_lifecycle_test.go @@ -0,0 +1,209 @@ +package database_test + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/fx" + "gorm.io/gorm" + "sneak.berlin/go/webhooker/internal/database" +) + +const ( + // reaperTestInterval is the sweep interval a lifecycle test + // runs the reaper at, so a loop that survives startup produces + // an observable sweep quickly. + reaperTestInterval = 10 * time.Millisecond + + // reaperStopTimeout bounds how long a lifecycle test waits for + // the reaper's OnStop hook to return before declaring the + // shutdown hung. + reaperStopTimeout = 10 * time.Second + + // reaperTestRetentionDays is the retention policy the lifecycle + // tests give their webhook. + reaperTestRetentionDays = 30 +) + +// recordingLifecycle is a minimal fx.Lifecycle that records the +// hooks a component registers, so a test can invoke the real +// OnStart/OnStop functions with a context of its choosing. +type recordingLifecycle struct { + hooks []fx.Hook +} + +func (l *recordingLifecycle) Append(h fx.Hook) { + l.hooks = append(l.hooks, h) +} + +// startReaperViaHook drives the genuine fx hooks the application +// registers for the reaper, handing OnStart a context that is +// already done. It returns the recorded lifecycle so the caller +// can drive OnStop too. +func startReaperViaHook( + t *testing.T, r *database.RetentionReaper, +) *recordingLifecycle { + t.Helper() + + lc := &recordingLifecycle{} + r.ExportRegisterHooks(lc) + require.Len(t, lc.hooks, 1) + + // fx hands OnStart a context carrying the application start + // timeout, and cancels it when the start phase ends. An + // already-cancelled context is that same defect taken to its + // limit, and unlike a plain context.Background() it actually + // distinguishes a correctly rooted loop from a broken one. + hookCtx, cancel := context.WithCancel(context.Background()) + cancel() + + require.NoError(t, lc.hooks[0].OnStart(hookCtx)) + + return lc +} + +// eventGone reports whether an event row has been removed. It +// takes no *testing.T because it is polled from an +// assert.Eventually condition, which runs off the test goroutine +// where testify assertions must not be used. +func eventGone(db *gorm.DB, eventID string) bool { + var n int64 + + err := db.Unscoped().Model(&database.Event{}). + Where("id = ?", eventID).Count(&n).Error + if err != nil { + return false + } + + return n == 0 +} + +// seedExpiredWebhook creates a webhook with a finite retention +// policy plus one long-expired event chain, and returns the +// webhook's database and the chain's event ID. +func seedExpiredWebhook( + t *testing.T, env *retentionTestEnv, +) (*gorm.DB, string) { + t.Helper() + + webhookID := createWebhook( + t, env.mainDB.DB(), reaperTestRetentionDays, + ) + + db, err := env.mgr.GetDB(webhookID) + require.NoError(t, err) + + chain := seedEventChain( + t, db, webhookID, + time.Now().Add(-365*24*time.Hour), + ) + + return db, chain.eventID +} + +// TestRetentionReaper_LoopOutlivesStartHookContext is the +// regression test for a reaper that never reaped. fx calls +// OnStart with a context carrying the application's start timeout +// (15s by default) and cancels it when the start phase ends, so a +// sweep loop rooted in it is dead three quarters of an hour +// before its first tick under the default one-hour interval, and +// per-webhook event databases grow without bound exactly as they +// did before retention existed. +// +// Driving OnStart with an already-cancelled context is that +// defect taken to its limit: a loop that inherits the hook +// context never ticks once, while a correctly rooted loop keeps +// sweeping for as long as the process lives. +func TestRetentionReaper_LoopOutlivesStartHookContext( + t *testing.T, +) { + t.Parallel() + + env := setupRetentionTest(t) + + db, eventID := seedExpiredWebhook(t, env) + + env.reaper.ExportSetInterval(reaperTestInterval) + + lc := startReaperViaHook(t, env.reaper) + t.Cleanup(func() { + _ = lc.hooks[0].OnStop(context.Background()) + }) + + assert.Eventually( + t, + func() bool { return eventGone(db, eventID) }, + 5*time.Second, + reaperTestInterval, + "the sweep loop must keep running after the start "+ + "hook's context is done; it reaped nothing, so it "+ + "inherited the hook context and died", + ) +} + +// TestRetentionReaper_StopHookStopsLoop proves the fix did not +// trade a startup bug for a shutdown hang: now that the sweep +// loop no longer observes the start hook's cancellation, OnStop +// is the only thing that can stop it, and it must both return +// promptly and actually leave the loop stopped. +func TestRetentionReaper_StopHookStopsLoop(t *testing.T) { + t.Parallel() + + env := setupRetentionTest(t) + + db, eventID := seedExpiredWebhook(t, env) + + env.reaper.ExportSetInterval(reaperTestInterval) + + lc := startReaperViaHook(t, env.reaper) + + // Let the loop prove it is running before stopping it, so a + // fast OnStop cannot pass by stopping something already dead. + require.Eventually( + t, + func() bool { return eventGone(db, eventID) }, + 5*time.Second, + reaperTestInterval, + ) + + var stopErr error + + stopped := make(chan struct{}) + + go func() { + defer close(stopped) + + // stop blocks on the loop's WaitGroup, so returning at all + // proves the goroutine observed the cancellation. + stopErr = lc.hooks[0].OnStop(context.Background()) + }() + + select { + case <-stopped: + case <-time.After(reaperStopTimeout): + t.Fatal( + "OnStop did not return: the retention reaper's " + + "WaitGroup is still waiting on a loop that never " + + "observed cancellation", + ) + } + + require.NoError(t, stopErr) + + // With the loop gone, a newly expired chain must survive. + survivor := seedEventChain( + t, db, "stopped-webhook", + time.Now().Add(-365*24*time.Hour), + ) + + time.Sleep(20 * reaperTestInterval) + + assert.False( + t, + eventGone(db, survivor.eventID), + "a stopped reaper must not sweep anything", + ) +} diff --git a/internal/database/retention_test.go b/internal/database/retention_test.go index 0ff0c90..c2dccef 100644 --- a/internal/database/retention_test.go +++ b/internal/database/retention_test.go @@ -77,7 +77,7 @@ func createWebhook( wh := &database.Webhook{ UserID: uuid.New().String(), - Name: "test-webhook", + Name: testWebhookName, RetentionDays: retentionDays, } require.NoError( @@ -85,10 +85,11 @@ func createWebhook( db.Omit(clause.Associations).Create(wh).Error, ) - // The RetentionDays column carries a GORM default of 30, so a - // zero (or negative) value passed to Create is replaced by that - // default. Force the requested value explicitly so the - // retain-forever (<= 0) path can be exercised. + // Webhook.BeforeSave rewrites a non-positive RetentionDays to the + // retain-forever sentinel, and the column's GORM default would + // otherwise substitute 30. Force the requested value with a + // column-level update so tests can plant legacy rows that predate + // the sentinel and still carry a literal 0 or negative value. require.NoError( t, db.Model(wh). @@ -98,6 +99,30 @@ func createWebhook( return wh.ID } +// createWebhookNormally inserts a webhook through the ordinary Create +// path, with no column-level forcing, so Webhook.BeforeSave applies +// exactly as it does in production. Passing 0 therefore yields a row +// holding the RetentionForeverDays sentinel. +func createWebhookNormally( + t *testing.T, + db *gorm.DB, + retentionDays int, +) string { + t.Helper() + + wh := &database.Webhook{ + UserID: uuid.New().String(), + Name: testWebhookName, + RetentionDays: retentionDays, + } + require.NoError( + t, + db.Omit(clause.Associations).Create(wh).Error, + ) + + return wh.ID +} + // eventChain is the set of row IDs seeded for a single event. type eventChain struct { eventID string @@ -256,12 +281,111 @@ func TestRetentionReaper_ReapsExpiredKeepsRecent(t *testing.T) { assertChainPresent(t, db, recent) } +// TestRetentionReaper_SkipsSentinelReapsFiniteInSameSweep covers the +// end-to-end retain-forever path: a webhook created the normal way with +// a requested retention of 0 lands on the RetentionForeverDays +// sentinel, and the reaper leaves its ancient events alone while still +// reaping a finite-retention webhook in the very same sweep. +func TestRetentionReaper_SkipsSentinelReapsFiniteInSameSweep( + t *testing.T, +) { + t.Parallel() + + env := setupRetentionTest(t) + + foreverID := createWebhookNormally(t, env.mainDB.DB(), 0) + + var stored database.Webhook + + require.NoError( + t, + env.mainDB.DB().Where("id = ?", foreverID). + First(&stored).Error, + ) + require.Equal( + t, + database.RetentionForeverDays, + stored.RetentionDays, + "a requested retention of 0 must persist as the sentinel", + ) + + finiteID := createWebhookNormally(t, env.mainDB.DB(), 30) + + foreverDB, err := env.mgr.GetDB(foreverID) + require.NoError(t, err) + + finiteDB, err := env.mgr.GetDB(finiteID) + require.NoError(t, err) + + ancient := time.Now().Add(-365 * 24 * time.Hour) + kept := seedEventChain(t, foreverDB, foreverID, ancient) + doomed := seedEventChain(t, finiteDB, finiteID, ancient) + + env.reaper.ExportSweep(context.Background()) + + assertChainPresent(t, foreverDB, kept) + assertChainGone(t, finiteDB, doomed) +} + +// TestRetentionReaper_HugeFiniteRetentionRetainsRecentEvents pins the +// overflow that made a large finite retention destroy everything. +// +// The cutoff is a time.Duration, an int64 nanosecond count. A day +// count above MaxFiniteRetentionDays multiplied out unclamped wraps +// negative, so subtracting it moves the cutoff into the far future, +// where "created_at < cutoff" matches every row: an event created a +// moment ago, and its delivery and delivery result, were all deleted +// on the first sweep. 200000 is inside that band and below the +// retain-forever sentinel, so it is treated as a finite policy and +// really does reach the arithmetic. +// +// The row is planted at the column level because such a value can no +// longer be submitted through the form; the point of the test is that +// a row from an older version, or a future call site, still cannot +// trigger the wipe. +func TestRetentionReaper_HugeFiniteRetentionRetainsRecentEvents( + t *testing.T, +) { + t.Parallel() + + env := setupRetentionTest(t) + + const overflowingRetentionDays = 200000 + + require.Greater( + t, + overflowingRetentionDays, + database.MaxFiniteRetentionDays, + "the test value must exceed what the cutoff can represent", + ) + require.Less( + t, + overflowingRetentionDays, + database.RetentionForeverDays, + "the test value must not be rescued by the forever skip", + ) + + webhookID := createWebhook( + t, env.mainDB.DB(), overflowingRetentionDays, + ) + + db, err := env.mgr.GetDB(webhookID) + require.NoError(t, err) + + fresh := seedEventChain(t, db, webhookID, time.Now()) + + env.reaper.ExportSweep(context.Background()) + + assertChainPresent(t, db, fresh) +} + func TestRetentionReaper_RetainsForeverWhenNonPositive(t *testing.T) { t.Parallel() env := setupRetentionTest(t) - // RetentionDays of zero means retain forever. + // A legacy row written before the sentinel existed still carries a + // literal 0; the <= 0 guard must keep honouring it. webhookID := createWebhook(t, env.mainDB.DB(), 0) db, err := env.mgr.GetDB(webhookID) diff --git a/internal/delivery/archive_sweeper.go b/internal/delivery/archive_sweeper.go new file mode 100644 index 0000000..c5b83f2 --- /dev/null +++ b/internal/delivery/archive_sweeper.go @@ -0,0 +1,229 @@ +package delivery + +import ( + "context" + "errors" + "log/slog" + "sync" + "time" + + "go.uber.org/fx" + "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/logger" +) + +// ArchiveSweeperParams holds the fx dependencies for the +// ArchiveSweeper. +type ArchiveSweeperParams struct { + fx.In + + Config *config.Config + Database *database.Database + Engine *Engine + Logger *logger.Logger +} + +// ArchiveSweeper periodically prunes expired rows from +// per-webhook archive databases whose database target carries a +// positive expiry. +// +// Without it, pruning happens only when an archive is +// (re)opened, and archives are only ever reopened by writes: an +// archive belonging to a webhook that has stopped receiving +// events would keep its expired rows forever. The sweep closes +// that gap without changing anything for archives whose expiry +// is unset or "never". +// +// It reuses Config.RetentionSweepInterval rather than +// introducing a second interval: this is a retention sweep with +// the same semantics as the event retention reaper. +type ArchiveSweeper struct { + db *database.Database + eng *Engine + log *slog.Logger + interval time.Duration + cancel context.CancelFunc + wg sync.WaitGroup +} + +// NewArchiveSweeper creates the archive sweeper and registers +// its fx lifecycle hooks. The background sweep loop starts on +// OnStart and stops cleanly on OnStop via context cancellation. +func NewArchiveSweeper( + lc fx.Lifecycle, + params ArchiveSweeperParams, +) *ArchiveSweeper { + s := &ArchiveSweeper{ + db: params.Database, + eng: params.Engine, + log: params.Logger.Get(), + interval: params.Config.RetentionSweepInterval, + } + + s.registerHooks(lc) + + return s +} + +// registerHooks wires the sweeper's start and stop into the fx +// lifecycle. Both hook contexts are deliberately ignored: see +// start for why the background loop must not inherit the start +// hook's context, and stop for why shutdown blocks on the loop +// rather than on the stop hook's deadline. +func (s *ArchiveSweeper) registerHooks(lc fx.Lifecycle) { + lc.Append(fx.Hook{ + //nolint:contextcheck // Not passing the hook context is + // the point: see start. + OnStart: func(_ context.Context) error { + s.start() + + return nil + }, + OnStop: func(_ context.Context) error { + s.stop() + + return nil + }, + }) +} + +// start launches the background sweep loop. +// +// The loop's context is derived from context.Background(), NOT +// from the fx OnStart hook context. The hook context carries +// fx's start timeout (15s by default), so a loop derived from it +// is cancelled 15 seconds after the application starts — long +// before the first tick under the default one-hour sweep +// interval, leaving a sweeper that never sweeps. A long-lived +// goroutine must outlive the startup phase, so its lifetime is +// bounded by OnStop instead: stop cancels this context and waits +// on the WaitGroup. +func (s *ArchiveSweeper) start() { + ctx, cancel := context.WithCancel(context.Background()) + s.cancel = cancel + + s.wg.Add(1) + + go s.run(ctx) + + s.log.Info( + "archive sweeper started", + "interval", s.interval.String(), + ) +} + +func (s *ArchiveSweeper) stop() { + s.log.Info("archive sweeper stopping") + + if s.cancel != nil { + s.cancel() + } + + s.wg.Wait() + s.log.Info("archive sweeper stopped") +} + +func (s *ArchiveSweeper) run(ctx context.Context) { + defer s.wg.Done() + + ticker := time.NewTicker(s.interval) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + s.sweep(ctx) + } + } +} + +// sweep prunes every archive whose database target declares a +// positive expiry. Targets belonging to a deleted webhook are +// soft-deleted along with it, so GORM's default scope already +// excludes them. +// +// A failure for one webhook is logged and the sweep continues, +// matching how the write path already treats a prune error as +// non-fatal. +func (s *ArchiveSweeper) sweep(ctx context.Context) { + var targets []database.Target + + err := s.db.DB(). + Model(&database.Target{}). + Where("type = ?", database.TargetTypeDatabase). + Find(&targets).Error + if err != nil { + s.log.Error( + "archive sweep: failed to list database targets", + "error", err, + ) + + return + } + + for i := range targets { + select { + case <-ctx.Done(): + return + default: + } + + s.sweepTarget(&targets[i]) + } +} + +// sweepTarget prunes the archive of a single database target. +// A missing, empty, or "never" expiry parses as a zero duration +// and is skipped entirely, so those archives keep exactly the +// behaviour they had before the sweep existed. +func (s *ArchiveSweeper) sweepTarget(target *database.Target) { + expiry, err := parseArchiveExpiry(target.Config) + if err != nil { + s.log.Error( + "archive sweep: invalid database target config", + "webhook_id", target.WebhookID, + "target_id", target.ID, + "error", err, + ) + + return + } + + if expiry <= 0 { + return + } + + if s.eng == nil || s.eng.dbTarget == nil { + return + } + + err = s.eng.dbTarget.sweepWebhook(target.WebhookID, expiry) + if err == nil { + return + } + + // A writer evicted underneath the sweep means the operator + // deleted the webhook (or its last database target) while the + // sweep was walking the target list. That is an ordinary + // interleaving, not a failure, so it must not produce an + // error line. + if errors.Is(err, errArchiveWriterEvicted) { + s.log.Debug( + "archive sweep: writer evicted mid-sweep", + "webhook_id", target.WebhookID, + "target_id", target.ID, + ) + + return + } + + s.log.Error( + "archive sweep: failed to prune archive", + "webhook_id", target.WebhookID, + "target_id", target.ID, + "error", err, + ) +} diff --git a/internal/delivery/archive_sweeper_test.go b/internal/delivery/archive_sweeper_test.go new file mode 100644 index 0000000..20f210d --- /dev/null +++ b/internal/delivery/archive_sweeper_test.go @@ -0,0 +1,930 @@ +package delivery_test + +import ( + "context" + "database/sql" + "fmt" + "net/http" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/fx" + "gorm.io/driver/sqlite" + "gorm.io/gorm" + "gorm.io/gorm/clause" + _ "modernc.org/sqlite" // Pure Go SQLite driver. + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/delivery" +) + +const ( + // sweepRowOld and sweepRowNew are the event ids + // seedArchiveRows assigns to the first and second seeded + // rows. + sweepRowOld = "ev-0" + sweepRowNew = "ev-1" + + // sweepConcurrentWrites is how many deliveries the + // concurrent write-plus-sweep test races against the sweep. + sweepConcurrentWrites = 20 +) + +// sweeperEnv bundles the pieces an archive sweep test drives: +// a main configuration database holding webhooks and targets, a +// delivery engine owning the archive writer registry, and the +// data directory the archive files live in. +type sweeperEnv struct { + sweeper *delivery.ArchiveSweeper + eng *delivery.Engine + mainDB *database.Database + dataDir string +} + +func setupSweeperTest(t *testing.T) *sweeperEnv { + t.Helper() + + dataDir := t.TempDir() + log := archiveTestLogger() + + sqlDB, err := sql.Open( + "sqlite", + fmt.Sprintf( + "file:%s?mode=rwc", + filepath.Join(dataDir, "main.db"), + ), + ) + require.NoError(t, err) + + t.Cleanup(func() { _ = sqlDB.Close() }) + + gdb, err := gorm.Open( + sqlite.Dialector{Conn: sqlDB}, &gorm.Config{}, + ) + require.NoError(t, err) + + mainDB := database.NewTestDatabase(gdb) + require.NoError(t, mainDB.Migrate()) + + eng := delivery.NewTestEngineWithDB( + mainDB, + database.NewTestWebhookDBManager(dataDir), + log, + &http.Client{Timeout: 5 * time.Second}, + 1, + ) + + return &sweeperEnv{ + sweeper: delivery.NewTestArchiveSweeper( + mainDB, eng, log, + ), + eng: eng, + mainDB: mainDB, + dataDir: dataDir, + } +} + +// archivePath returns where the engine keeps a webhook's +// archive file. +func (env *sweeperEnv) archivePath(webhookID string) string { + return filepath.Join( + env.dataDir, fmt.Sprintf("archive-%s.db", webhookID), + ) +} + +// seedDatabaseTarget creates a webhook with one database target +// carrying the given target config JSON, and returns the +// webhook id. +func (env *sweeperEnv) seedDatabaseTarget( + t *testing.T, configJSON string, +) string { + t.Helper() + + wh := &database.Webhook{ + UserID: uuid.New().String(), + Name: "sweep-test", + } + require.NoError( + t, + env.mainDB.DB(). + Omit(clause.Associations). + Create(wh).Error, + ) + + tgt := &database.Target{ + WebhookID: wh.ID, + Name: "archive", + Type: database.TargetTypeDatabase, + Active: true, + Config: configJSON, + } + require.NoError( + t, + env.mainDB.DB(). + Omit(clause.Associations). + Create(tgt).Error, + ) + + return wh.ID +} + +// seedArchiveRows creates the archive file for a webhook and +// inserts one row per supplied archived-at timestamp, returning +// the archive path. The handle is closed before returning, so +// the archive is idle exactly as it would be with no traffic. +func (env *sweeperEnv) seedArchiveRows( + t *testing.T, webhookID string, archivedAt ...time.Time, +) string { + t.Helper() + + path := env.archivePath(webhookID) + + sqlDB, err := sql.Open( + "sqlite", fmt.Sprintf("file:%s?mode=rwc", path), + ) + require.NoError(t, err) + + gdb, err := gorm.Open( + sqlite.Dialector{Conn: sqlDB}, &gorm.Config{}, + ) + require.NoError(t, err) + + require.NoError( + t, gdb.AutoMigrate(&delivery.ExportArchivedEvent{}), + ) + + for i, at := range archivedAt { + row := delivery.ExportArchivedEvent{ + EventID: fmt.Sprintf("ev-%d", i), + WebhookID: webhookID, + Method: http.MethodPost, + Body: `{"seeded":true}`, + ArchivedAt: at, + } + require.NoError(t, gdb.Create(&row).Error) + } + + require.NoError(t, sqlDB.Close()) + + return path +} + +// archivedEventIDs returns the event ids currently stored in an +// archive file, read through a separate read-only handle. +func archivedEventIDs( + t *testing.T, path string, +) []string { + t.Helper() + + var rows []delivery.ExportArchivedEvent + + rdb := openArchiveDBForRead(t, path) + require.NoError(t, rdb.Order("event_id").Find(&rows).Error) + + ids := make([]string, 0, len(rows)) + for i := range rows { + ids = append(ids, rows[i].EventID) + } + + return ids +} + +// countArchivedRows counts the rows in an archive file without +// asserting anything, so it is safe to poll from an +// assert.Eventually condition (which runs off the test +// goroutine, where testify assertions must not be used). +func countArchivedRows(path string) (int64, error) { + sqlDB, err := sql.Open( + "sqlite", fmt.Sprintf("file:%s?mode=ro", path), + ) + if err != nil { + return 0, err + } + + defer func() { _ = sqlDB.Close() }() + + gdb, err := gorm.Open( + sqlite.Dialector{Conn: sqlDB}, &gorm.Config{}, + ) + if err != nil { + return 0, err + } + + var count int64 + + err = gdb.Model(&delivery.ExportArchivedEvent{}). + Count(&count).Error + if err != nil { + return 0, err + } + + return count, nil +} + +// captureLifecycle is a minimal fx.Lifecycle that records the +// hooks a component registers, so a test can invoke the real +// OnStart/OnStop functions with a context of its choosing. +type captureLifecycle struct { + hooks []fx.Hook +} + +func (l *captureLifecycle) Append(h fx.Hook) { + l.hooks = append(l.hooks, h) +} + +// TestArchiveSweeper_LoopOutlivesStartHookContext is the +// regression test for a sweeper that never swept. fx calls +// OnStart with a context carrying the application's start +// timeout (15 seconds by default), so a background loop whose +// context is derived from it is cancelled 15 seconds into the +// process — three quarters of an hour before the first tick +// under the default one-hour sweep interval. +// +// The hook context here is already cancelled, which is the same +// defect taken to its limit: a loop that inherits it never runs +// a single tick, while a correctly rooted loop keeps sweeping +// for as long as the process lives. Handing the hook a plain +// context.Background() would assert nothing at all. +func TestArchiveSweeper_LoopOutlivesStartHookContext( + t *testing.T, +) { + t.Parallel() + + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + + now := time.Now() + path := env.seedArchiveRows( + t, webhookID, + now.Add(-48*time.Hour), + now.Add(-time.Minute), + ) + + env.sweeper.ExportSetInterval(10 * time.Millisecond) + + // Drive the genuine fx hooks the application registers, + // rather than a test-only entry point. + lc := &captureLifecycle{} + env.sweeper.ExportRegisterHooks(lc) + require.Len(t, lc.hooks, 1) + + hookCtx, cancel := context.WithCancel(context.Background()) + cancel() + + require.NoError(t, lc.hooks[0].OnStart(hookCtx)) + + t.Cleanup(func() { + _ = lc.hooks[0].OnStop(context.Background()) + }) + + assert.Eventually( + t, + func() bool { + count, err := countArchivedRows(path) + + return err == nil && count == 1 + }, + 5*time.Second, + 10*time.Millisecond, + "the sweep loop must keep running after the start "+ + "hook's context is done; it pruned nothing, so it "+ + "inherited the hook context and died", + ) +} + +// TestArchiveSweep_DoesNotResurrectEvictedWriter covers the +// interleaving where a sweep tick has already listed a webhook's +// target when the webhook is deleted and its writer evicted. The +// sweep must not put a writer back into the registry: nothing +// would ever evict it again, which is precisely the leak this +// change exists to close. +func TestArchiveSweep_DoesNotResurrectEvictedWriter( + t *testing.T, +) { + t.Parallel() + + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + env.seedArchiveRows( + t, webhookID, time.Now().Add(-48*time.Hour), + ) + + // Prime the registry the way a delivery would, then evict as + // the deletion path does. The target row is deliberately left + // in place: this is the tick that listed the webhook before + // the deletion committed. + _, err := env.eng.ExportEnsureArchiveWriter(webhookID) + require.NoError(t, err) + + env.eng.EvictWebhook(webhookID) + require.False(t, env.eng.ExportHasArchiveWriter(webhookID)) + + env.sweeper.ExportSweep(context.Background()) + + assert.False( + t, env.eng.ExportHasArchiveWriter(webhookID), + "a sweep must never re-register a writer for a webhook "+ + "whose registry entry has already been released", + ) +} + +// TestArchiveSweep_LeavesNoRegistryEntry states the same +// invariant in its general form: sweeping an archive whose +// webhook has no cached writer must not leave one behind, so the +// registry keeps holding only writers a delivery created and an +// eviction can reach. +func TestArchiveSweep_LeavesNoRegistryEntry(t *testing.T) { + t.Parallel() + + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + path := env.seedArchiveRows( + t, webhookID, + time.Now().Add(-48*time.Hour), + time.Now().Add(-time.Minute), + ) + + require.False(t, env.eng.ExportHasArchiveWriter(webhookID)) + + env.sweeper.ExportSweep(context.Background()) + + assert.Equal( + t, []string{sweepRowNew}, archivedEventIDs(t, path), + "the sweep must still prune an idle archive", + ) + assert.False( + t, env.eng.ExportHasArchiveWriter(webhookID), + "the sweep must release the registry entry it created", + ) +} + +// TestArchiveSweep_KeepsWriterAdoptedByDelivery is the other +// half of that invariant: an entry the sweep created but a +// delivery then claimed belongs to the registry and must survive +// the sweep, or the delivery would be left holding a detached +// writer with an open handle that no eviction can reach. +func TestArchiveSweep_KeepsWriterAdoptedByDelivery( + t *testing.T, +) { + t.Parallel() + + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + env.seedArchiveRows( + t, webhookID, time.Now().Add(-48*time.Hour), + ) + + webhookDB := testWebhookDB(t) + event := seedEvent(t, webhookDB, `{"n":1}`) + event.WebhookID = webhookID + d := seedDatabaseTargetDelivery( + t, webhookDB, event, `{"expiry":"1h"}`, + ) + + env.sweeper.ExportSweep(context.Background()) + require.False(t, env.eng.ExportHasArchiveWriter(webhookID)) + + env.eng.ExportDeliverDatabase(webhookDB, d) + + assert.True( + t, env.eng.ExportHasArchiveWriter(webhookID), + "a delivery's writer must stay registered", + ) + + env.sweeper.ExportSweep(context.Background()) + + assert.True( + t, env.eng.ExportHasArchiveWriter(webhookID), + "a sweep must not drop a writer a delivery owns", + ) +} + +// TestArchiveSweep_KeepsWriterAdoptedDuringSweep covers the one +// interleaving the sweepOwned flag exists for, which +// TestArchiveSweep_KeepsWriterAdoptedByDelivery cannot reach: a +// delivery adopting the sweep's own entry WHILE that sweep is +// still running. +// +// The registry operations are driven directly, in the order the +// sweep and a concurrent delivery perform them, so the window is +// exercised deterministically rather than hoped for: +// +// 1. the sweep finds no cached writer and registers one of its +// own, marked sweep-owned; +// 2. a delivery arrives, is handed that very writer, clears the +// flag and opens the archive handle; +// 3. the sweep finishes and releases what it created. +// +// Step 3 must leave the entry alone. Dropping it would detach a +// writer that is holding an open archive handle inside its +// debounce window, and no eviction could ever reach it again — +// exactly the process-lifetime handle leak this change exists to +// close. The eviction at the end proves the entry is still +// reachable. +func TestArchiveSweep_KeepsWriterAdoptedDuringSweep( + t *testing.T, +) { + t.Parallel() + + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + env.seedArchiveRows( + t, webhookID, time.Now().Add(-48*time.Hour), + ) + + sweepWriter, created, err := env.eng.ExportSweepWriterFor( + webhookID, + ) + require.NoError(t, err) + require.True( + t, created, + "the sweep must have created the registry entry itself", + ) + + // The delivery lands mid-sweep and adopts the entry. + webhookDB := testWebhookDB(t) + event := seedEvent(t, webhookDB, `{"n":1}`) + event.WebhookID = webhookID + d := seedDatabaseTargetDelivery( + t, webhookDB, event, `{"expiry":"1h"}`, + ) + + env.eng.ExportDeliverDatabase(webhookDB, d) + + adopted := env.eng.ExportArchiveWriterFor(webhookID) + require.NotNil(t, adopted) + require.True( + t, sweepWriter.Same(adopted), + "the delivery must have adopted the sweep's writer", + ) + require.True( + t, env.eng.ExportArchiveHandleOpen(webhookID), + "the delivery leaves the archive handle open", + ) + + // The sweep finishes. + env.eng.ExportReleaseSweepWriter(webhookID, sweepWriter) + + require.True( + t, env.eng.ExportHasArchiveWriter(webhookID), + "a writer adopted by a delivery during a sweep must "+ + "stay registered, or its open handle is unreachable", + ) + + env.eng.EvictWebhook(webhookID) + + assert.False( + t, env.eng.ExportHasArchiveWriter(webhookID), + "the adopted writer must still be evictable", + ) + assert.False( + t, sweepWriter.HandleOpen(), + "eviction must have closed the adopted writer's handle", + ) +} + +// TestArchiveSweep_ContinuesAfterPerWebhookFailure proves a +// failure for one webhook does not abort the sweep for the +// others: an unparseable expiry and an unreadable archive both +// have to be logged and stepped over. +func TestArchiveSweep_ContinuesAfterPerWebhookFailure( + t *testing.T, +) { + t.Parallel() + + env := setupSweeperTest(t) + + // Seeded first so the sweep reaches them before the healthy + // webhook: targets come back in insertion order. + badConfigID := env.seedDatabaseTarget(t, `{"expiry":"!!!"}`) + env.seedArchiveRows( + t, badConfigID, time.Now().Add(-48*time.Hour), + ) + + corruptID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + require.NoError(t, os.WriteFile( + env.archivePath(corruptID), + []byte("this is not a sqlite database"), + 0o600, + )) + + healthyID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + healthyPath := env.seedArchiveRows( + t, healthyID, + time.Now().Add(-48*time.Hour), + time.Now().Add(-time.Minute), + ) + + env.sweeper.ExportSweep(context.Background()) + + assert.Equal( + t, []string{sweepRowNew}, + archivedEventIDs(t, healthyPath), + "a failure for an earlier webhook must not stop the "+ + "sweep from pruning the ones after it", + ) +} + +// TestArchiveSweep_OpenExistingDoesNotCreateFile pins the second +// of the two no-create guards. The first is the stat in +// sweepWebhook; this one is the SQLite open mode, which is what +// protects the window between that stat and the open. Flipping +// the sweep's mode to create-if-missing makes this fail. +func TestArchiveSweep_OpenExistingDoesNotCreateFile( + t *testing.T, +) { + t.Parallel() + + dir := t.TempDir() + path := filepath.Join(dir, "archive-absent.db") + + w := delivery.NewExportArchiveWriter( + path, archiveTestLogger(), 0, + ) + + err := w.OpenExisting(time.Hour) + + require.Error( + t, err, + "opening a missing archive without create permission "+ + "must fail rather than conjure the file", + ) + + for _, suffix := range archiveFileSuffixes() { + assert.NoFileExists(t, path+suffix) + } +} + +// TestArchiveSweep_PrunesIdleArchive is the core regression +// test for this issue: an archive that receives no further +// writes must still lose its expired rows. Before the sweeper +// existed, pruning only ever ran on a write-triggered reopen, +// so an idle archive kept expired rows forever. +func TestArchiveSweep_PrunesIdleArchive(t *testing.T) { + t.Parallel() + + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + + now := time.Now() + path := env.seedArchiveRows( + t, webhookID, + now.Add(-48*time.Hour), + now.Add(-time.Minute), + ) + + require.Equal( + t, []string{sweepRowOld, sweepRowNew}, + archivedEventIDs(t, path), + ) + + env.sweeper.ExportSweep(context.Background()) + + assert.Equal( + t, []string{sweepRowNew}, archivedEventIDs(t, path), + "the sweep should prune rows older than the expiry "+ + "from an idle archive and keep the rest", + ) +} + +// TestArchiveSweep_LeavesArchiveClosed proves the sweep does +// not hold the archive open afterwards, so an operator can +// still move the file away for offline retention. +// +// The assertion is made on a writer the test holds a reference +// to, and the handle is proven OPEN before the sweep runs, so the +// test observes the sweep closing it rather than a writer that +// merely never opened anything. Asking the registry instead would +// be vacuous here: the sweep releases an entry it created, and a +// missing entry reports "not open" whether or not anything was +// closed. +func TestArchiveSweep_LeavesArchiveClosed(t *testing.T) { + t.Parallel() + + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + path := env.seedArchiveRows( + t, webhookID, time.Now().Add(-48*time.Hour), + ) + + w := delivery.NewExportArchiveWriter( + path, archiveTestLogger(), 0, + ) + + require.NoError(t, w.OpenExisting(time.Hour)) + require.True( + t, w.HandleOpen(), + "the writer must hold an open handle before the sweep", + ) + + require.NoError(t, w.SweepExpired(time.Hour)) + + assert.False( + t, w.HandleOpen(), + "an idle archive must end the sweep closed", + ) +} + +// TestArchiveSweep_ClosesHandleOfRegisteredWriter states the same +// guarantee end to end, through the real sweeper and a writer the +// registry keeps. +// +// The delivery leaves the archive handle open inside its debounce +// window and makes the entry delivery-owned, so the sweep finds a +// cached writer (created is false, nothing is released) and the +// registry query afterwards is answered by a writer that really +// exists. A handle left open here would be doubly wrong: it also +// blocks the operator's move-the-file-away workflow. +func TestArchiveSweep_ClosesHandleOfRegisteredWriter( + t *testing.T, +) { + t.Parallel() + + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + env.seedArchiveRows( + t, webhookID, time.Now().Add(-48*time.Hour), + ) + + webhookDB := testWebhookDB(t) + event := seedEvent(t, webhookDB, `{"n":1}`) + event.WebhookID = webhookID + d := seedDatabaseTargetDelivery( + t, webhookDB, event, `{"expiry":"1h"}`, + ) + + env.eng.ExportDeliverDatabase(webhookDB, d) + + require.True( + t, env.eng.ExportArchiveHandleOpen(webhookID), + "the delivery must leave the archive handle open", + ) + + env.sweeper.ExportSweep(context.Background()) + + require.True( + t, env.eng.ExportHasArchiveWriter(webhookID), + "the delivery's registry entry must survive the sweep", + ) + assert.False( + t, env.eng.ExportArchiveHandleOpen(webhookID), + "the sweep must leave the archive closed", + ) +} + +// TestArchiveSweep_NeverExpiryUntouched proves the sweep is a +// no-op for the default retention policy, so archives with no +// expiry (or the literal "never") behave exactly as before. +func TestArchiveSweep_NeverExpiryUntouched(t *testing.T) { + t.Parallel() + + for _, configJSON := range []string{ + `{"expiry":"never"}`, + `{"expiry":""}`, + "", + } { + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, configJSON) + path := env.seedArchiveRows( + t, webhookID, + time.Now().Add(-10000*time.Hour), + ) + + env.sweeper.ExportSweep(context.Background()) + + assert.Equal( + t, []string{sweepRowOld}, archivedEventIDs(t, path), + "config %q must keep rows forever", configJSON, + ) + assert.False( + t, env.eng.ExportHasArchiveWriter(webhookID), + "config %q must leave no registry entry behind", + configJSON, + ) + } +} + +// TestArchiveSweep_NeverExpirySkipsBeforeOpening pins the +// expiry <= 0 boundary in sweepTarget, which the row assertions +// above cannot reach: pruning is separately gated on a positive +// expiry, so a "never" archive keeps its rows even if the sweep +// does open it. +// +// The spec is stronger than that — a "never" archive is skipped +// before any file is touched — so the archive here exists but has +// never been migrated. Opening it at all would run AutoMigrate +// and create the archive table, which is exactly what must not +// happen. +func TestArchiveSweep_NeverExpirySkipsBeforeOpening( + t *testing.T, +) { + t.Parallel() + + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, `{"expiry":"never"}`) + path := env.archivePath(webhookID) + + seedUnmigratedArchive(t, path) + require.False(t, archiveTableExists(t, path)) + + env.sweeper.ExportSweep(context.Background()) + + assert.False( + t, archiveTableExists(t, path), + "a never-expiry archive must not be opened at all", + ) +} + +// seedUnmigratedArchive creates an archive file that exists but +// carries no archive schema, so any open of it is observable: the +// archive table appears only if something ran AutoMigrate. +func seedUnmigratedArchive(t *testing.T, path string) { + t.Helper() + + sqlDB, err := sql.Open( + "sqlite", fmt.Sprintf("file:%s?mode=rwc", path), + ) + require.NoError(t, err) + + _, err = sqlDB.ExecContext( + t.Context(), "CREATE TABLE placeholder (id INTEGER)", + ) + require.NoError(t, err) + + require.NoError(t, sqlDB.Close()) +} + +// archiveTableExists reports whether an archive file has had the +// archive schema migrated into it. +func archiveTableExists(t *testing.T, path string) bool { + t.Helper() + + return openArchiveDBForRead(t, path). + Migrator(). + HasTable(&delivery.ExportArchivedEvent{}) +} + +// TestArchiveSweep_DoesNotCreateArchiveFile proves the sweep +// never conjures an archive: a webhook with a database target +// that has never received an event must still have no archive +// file (nor SQLite sidecar) after a sweep. +func TestArchiveSweep_DoesNotCreateArchiveFile(t *testing.T) { + t.Parallel() + + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + path := env.archivePath(webhookID) + + require.NoFileExists(t, path) + + env.sweeper.ExportSweep(context.Background()) + + for _, suffix := range archiveFileSuffixes() { + assert.NoFileExists( + t, path+suffix, + "the sweep must not create an archive file", + ) + } +} + +// TestArchiveSweep_DoesNotCreateAfterWriterExists covers the +// same guarantee once a writer is cached in the registry but +// the file itself is still absent (for instance because the +// operator moved the archive away). +func TestArchiveSweep_DoesNotCreateAfterWriterExists( + t *testing.T, +) { + t.Parallel() + + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + + path, err := env.eng.ExportEnsureArchiveWriter(webhookID) + require.NoError(t, err) + require.NoFileExists(t, path) + + env.sweeper.ExportSweep(context.Background()) + + assert.NoFileExists(t, path) +} + +// TestArchiveSweep_SkipsDeletedWebhookTargets proves that the +// sweep ignores targets soft-deleted along with their webhook, +// so a deleted webhook's archive is never reopened. +func TestArchiveSweep_SkipsDeletedWebhookTargets(t *testing.T) { + t.Parallel() + + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + path := env.seedArchiveRows( + t, webhookID, time.Now().Add(-48*time.Hour), + ) + + require.NoError( + t, + env.mainDB.DB(). + Where("webhook_id = ?", webhookID). + Delete(&database.Target{}).Error, + ) + + env.sweeper.ExportSweep(context.Background()) + + assert.Equal( + t, []string{sweepRowOld}, archivedEventIDs(t, path), + "a deleted target's archive must be left alone", + ) +} + +// TestArchiveSweep_ConcurrentWrites proves the sweep serialises +// against writes through the per-webhook writer mutex. Run +// under -race, an unsynchronised sweep would be caught here. +func TestArchiveSweep_ConcurrentWrites(t *testing.T) { + t.Parallel() + + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + + webhookDB := testWebhookDB(t) + + // The deliveries are seeded up front, on the test's own + // goroutine: the seed helpers assert, and testify assertions + // must not run off the test goroutine. + deliveries := make( + []*database.Delivery, 0, sweepConcurrentWrites, + ) + + for range sweepConcurrentWrites { + event := seedEvent(t, webhookDB, `{"n":1}`) + event.WebhookID = webhookID + + deliveries = append( + deliveries, + seedDatabaseTargetDelivery( + t, webhookDB, event, `{"expiry":"1h"}`, + ), + ) + } + + var wg sync.WaitGroup + + wg.Add(2) + + go func() { + defer wg.Done() + + for _, d := range deliveries { + env.eng.ExportDeliverDatabase(webhookDB, d) + } + }() + + go func() { + defer wg.Done() + + for range sweepConcurrentWrites { + env.sweeper.ExportSweep(context.Background()) + } + }() + + wg.Wait() + + assert.FileExists(t, env.archivePath(webhookID)) +} + +// TestArchiveSweeper_StopsCleanly proves the background loop +// exits on OnStop rather than leaking a goroutine. +func TestArchiveSweeper_StopsCleanly(t *testing.T) { + t.Parallel() + + env := setupSweeperTest(t) + + webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) + env.seedArchiveRows( + t, webhookID, time.Now().Add(-48*time.Hour), + ) + + env.sweeper.ExportSetInterval(time.Millisecond) + env.sweeper.ExportStart() + + // stop blocks on the loop's WaitGroup, so returning at all + // proves the loop observed the cancellation and exited. + env.sweeper.ExportStop() +} diff --git a/internal/delivery/engine.go b/internal/delivery/engine.go index 6011558..564fd87 100644 --- a/internal/delivery/engine.go +++ b/internal/delivery/engine.go @@ -94,6 +94,23 @@ type Notifier interface { Notify(tasks []Task) } +// WebhookEvictor releases the delivery engine's per-webhook +// state for a webhook that no longer needs it — currently the +// cached archive writer of the database target, whose open +// file handle would otherwise outlive the webhook. +// +// It is deliberately separate from Notifier and deliberately +// one method wide: archiving lifecycle is not notification, and +// a single-method interface keeps the handlers package free of +// any dependency on the engine's internals while staying +// trivially fakeable in tests. +// +// EvictWebhook never deletes an archive file. It is idempotent +// and is a no-op for a webhook with no engine state. +type WebhookEvictor interface { + EvictWebhook(webhookID string) +} + // EngineParams are the fx dependencies for the delivery // engine. type EngineParams struct { @@ -127,6 +144,10 @@ type Engine struct { // httpTarget is retained so tests can reach the HTTP // target's shared client and circuit breakers. httpTarget *httpTarget + + // dbTarget is retained so the engine can reach the archive + // writer registry for webhook eviction and the idle sweep. + dbTarget *databaseTarget } // New creates and registers the delivery engine with the @@ -149,18 +170,7 @@ func New( Transport: NewSSRFSafeTransport(), }) - lc.Append(fx.Hook{ - OnStart: func(ctx context.Context) error { - e.start(ctx) - - return nil - }, - OnStop: func(_ context.Context) error { - e.stop() - - return nil - }, - }) + e.registerHooks(lc) return e } @@ -182,6 +192,19 @@ func (e *Engine) Notify(tasks []Task) { } } +// EvictWebhook implements WebhookEvictor. It releases the +// engine's per-webhook archiving state: the database target's +// cached archive writer is dropped from the registry and its +// file handle closed. The archive file itself is left on disk +// — it is long-term storage the operator owns. +func (e *Engine) EvictWebhook(webhookID string) { + if e.dbTarget == nil { + return + } + + e.dbTarget.evict(webhookID) +} + // ScheduleRetry schedules a task to be re-enqueued onto the // retry channel after delay. It implements the Scheduler // interface the targets use to own their durable retries. @@ -210,8 +233,40 @@ func (e *Engine) ScheduleRetry( }) } -func (e *Engine) start(ctx context.Context) { - ctx, cancel := context.WithCancel(ctx) +// registerHooks wires the engine's start and stop into the fx +// lifecycle. The start hook's context is deliberately ignored: +// see start for why the worker pool must not inherit it. +func (e *Engine) registerHooks(lc fx.Lifecycle) { + lc.Append(fx.Hook{ + //nolint:contextcheck // Not inheriting the hook context + // is the point: see start. + OnStart: func(_ context.Context) error { + e.start() + + return nil + }, + OnStop: func(_ context.Context) error { + e.stop() + + return nil + }, + }) +} + +// start launches the worker pool, restart recovery, and the +// periodic retry sweep. +// +// Their context is derived from context.Background(), NOT from +// the fx OnStart hook context. The hook context carries fx's +// start timeout (15s by default) and is cancelled once the start +// phase completes, so goroutines derived from it stop a few +// seconds into the process: every worker would return and the +// engine would silently stop delivering webhooks entirely. A +// long-lived goroutine must outlive the startup phase, so its +// lifetime is bounded by OnStop instead: stop cancels this +// context and waits on the WaitGroup. +func (e *Engine) start() { + ctx, cancel := context.WithCancel(context.Background()) e.cancel = cancel for range e.workers { @@ -453,8 +508,9 @@ func (e *Engine) recoverRetryingDeliveries( // recoverSingleRetry hands an orphaned retrying delivery back // to its target to recompute the remaining backoff, then // reschedules it. Targets that do not own durable retries -// (fire-and-forget) never produce retrying deliveries, so -// they are skipped. +// (fire-and-forget) never produce retrying deliveries, so a +// delivery found in that state has had its target's type +// changed underneath it and is terminally failed. func (e *Engine) recoverSingleRetry( webhookDB *gorm.DB, webhookID string, @@ -475,6 +531,10 @@ func (e *Engine) recoverSingleRetry( rs, ok := e.targets[target.Type].(rescheduler) if !ok { + e.failUnretryableRetry( + webhookDB, webhookID, d, &target, + ) + return } @@ -649,8 +709,8 @@ func (e *Engine) sweepWebhookRetries( // sweepSingleRetry re-enqueues an orphaned retrying delivery // whose backoff window has elapsed, delegating the backoff -// decision to the delivery's target. Targets that do not own -// durable retries are skipped. +// decision to the delivery's target. A delivery whose target +// no longer owns durable retries is terminally failed. func (e *Engine) sweepSingleRetry( webhookDB *gorm.DB, webhookID string, @@ -670,6 +730,10 @@ func (e *Engine) sweepSingleRetry( rs, ok := e.targets[target.Type].(rescheduler) if !ok { + e.failUnretryableRetry( + webhookDB, webhookID, d, &target, + ) + return } @@ -710,6 +774,59 @@ func (e *Engine) sweepSingleRetry( } } +// failUnretryableRetry terminally fails an orphaned retrying +// delivery whose target type no longer supports retries. Both +// restart recovery and the periodic sweep call it, so the +// terminal transition exists once. +// +// This is only reachable when a target's type has been changed +// out from under an in-flight retrying delivery (or the type is +// unknown to the registry): fire-and-forget targets never set +// 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. +func (e *Engine) failUnretryableRetry( + webhookDB *gorm.DB, + webhookID string, + d *database.Delivery, + target *database.Target, +) { + e.log.Warn( + "failing orphaned retrying delivery: target "+ + "type no longer supports retries", + "webhook_id", webhookID, + "delivery_id", d.ID, + "target_id", target.ID, + "target_name", target.Name, + "target_type", target.Type, + ) + + reason := fmt.Sprintf( + "target type %q does not support retries; "+ + "delivery was left retrying by a previous "+ + "target type and has been failed terminally", + target.Type, + ) + + e.recordResult( + webhookDB, + d, + e.countAttempts(webhookDB, d.ID)+1, + false, + 0, + "", + reason, + 0, + ) + + e.updateDeliveryStatus( + webhookDB, d, database.DeliveryStatusFailed, + ) +} + // processDelivery dispatches a delivery to the target that // owns its type. Unknown target types fail the delivery. func (e *Engine) processDelivery( diff --git a/internal/delivery/engine_integration_test.go b/internal/delivery/engine_integration_test.go index 6b07c43..50aeb1a 100644 --- a/internal/delivery/engine_integration_test.go +++ b/internal/delivery/engine_integration_test.go @@ -476,7 +476,7 @@ func TestWorkerLifecycle_StartStop(t *testing.T) { t.Parallel() s := newISetup(t) - s.Engine.ExportStart(context.Background()) + s.Engine.ExportStart() event := iSeedEvent( t, s.WebhookDB, s.WebhookID, @@ -499,21 +499,17 @@ func TestWorkerLifecycle_StartStop(t *testing.T) { s.Engine.Notify([]delivery.Task{task}) - iWaitForStatus( - t, s.WebhookDB, d.ID, - database.DeliveryStatusDelivered, - ) + iWaitForDelivered(t, s.WebhookDB, d.ID) s.Engine.ExportStop() } -// iWaitForStatus polls until the delivery reaches the -// expected status. -func iWaitForStatus( +// iWaitForDelivered polls until the delivery reaches the +// delivered status. +func iWaitForDelivered( t *testing.T, db *gorm.DB, deliveryID string, - expected database.DeliveryStatus, ) { t.Helper() @@ -527,7 +523,7 @@ func iWaitForStatus( return false } - return d.Status == expected + return d.Status == database.DeliveryStatusDelivered }, 5*time.Second, 50*time.Millisecond) } @@ -558,7 +554,7 @@ func TestWorkerLifecycle_ProcessesRetryChannel( database.DeliveryStatusRetrying, ) - s.Engine.ExportStart(context.Background()) + s.Engine.ExportStart() bodyStr := event.Body cfg := iHTTPConfig(ts.URL) @@ -569,10 +565,7 @@ func TestWorkerLifecycle_ProcessesRetryChannel( s.Engine.ExportRetryCh() <- task - iWaitForStatus( - t, s.WebhookDB, d.ID, - database.DeliveryStatusDelivered, - ) + iWaitForDelivered(t, s.WebhookDB, d.ID) s.Engine.ExportStop() } @@ -748,6 +741,193 @@ func TestRecoverWebhookDeliveries_RetryingDeliveries( case <-time.After(5 * time.Second): t.Fatal("expected retry task from recovery") } + + // Regression guard: a target that still supports retries + // must be rescheduled, never terminally failed, and must + // not gain a synthetic result row. + iAssertStatus( + t, s.WebhookDB, d.ID, + database.DeliveryStatusRetrying, + ) + + assert.Len(t, iResults(t, s.WebhookDB, d.ID), 1) +} + +// --- Retrying deliveries whose target type changed --- + +// iSeedRetryingWithType seeds a retrying delivery with one +// recorded failed attempt against a target of the given type, +// standing in for a target whose type was edited in the main +// database while the delivery was still retrying. +func iSeedRetryingWithType( + t *testing.T, + s iSetup, + targetType database.TargetType, +) string { + t.Helper() + + targetID := uuid.New().String() + + iCreateTarget(t, s.MainDB, targetID, + s.WebhookID, "mutated-target", targetType, + iHTTPConfig("http://example.com/hook"), 5, + ) + + event := iSeedEvent( + t, s.WebhookDB, s.WebhookID, + `{"orphaned":"retry"}`, + ) + + d := iSeedDelivery( + t, s.WebhookDB, event.ID, targetID, + database.DeliveryStatusRetrying, + ) + + iSeedFailedResult(t, s.WebhookDB, d.ID) + + return d.ID +} + +// iResults loads a delivery's results in attempt order. +func iResults( + t *testing.T, db *gorm.DB, deliveryID string, +) []database.DeliveryResult { + t.Helper() + + var results []database.DeliveryResult + + require.NoError(t, db. + Where("delivery_id = ?", deliveryID). + Order("attempt_num"). + Find(&results).Error) + + return results +} + +// iAssertTerminallyFailed asserts the delivery ended failed +// with a result row recording why, and was not rescheduled. +func iAssertTerminallyFailed( + t *testing.T, + s iSetup, + deliveryID string, + targetType database.TargetType, +) { + t.Helper() + + iAssertStatus( + t, s.WebhookDB, deliveryID, + database.DeliveryStatusFailed, + ) + + results := iResults(t, s.WebhookDB, deliveryID) + require.Len(t, results, 2) + + last := results[1] + + assert.False(t, last.Success) + assert.Equal(t, 2, last.AttemptNum) + + assert.Contains( + t, last.Error, string(targetType), + ) + + assert.Contains( + t, last.Error, "does not support retries", + ) + + assert.Empty(t, s.Engine.ExportRetryCh()) +} + +func TestRecoverSingleRetry_TypeNoLongerRetries( + t *testing.T, +) { + t.Parallel() + + s := newISetup(t) + + iCreateWebhook( + t, s.MainDB, s.WebhookID, "mutated-type", + ) + + deliveryID := iSeedRetryingWithType( + t, s, database.TargetTypeLog, + ) + + s.Engine.ExportRecoverWebhookDeliveries( + context.Background(), s.WebhookID, + ) + + iAssertTerminallyFailed( + t, s, deliveryID, database.TargetTypeLog, + ) +} + +func TestSweepSingleRetry_TypeNoLongerRetries( + t *testing.T, +) { + t.Parallel() + + s := newISetup(t) + + iCreateWebhook( + t, s.MainDB, s.WebhookID, "mutated-type-sweep", + ) + + deliveryID := iSeedRetryingWithType( + t, s, database.TargetTypeDatabase, + ) + + s.Engine.ExportSweepWebhookRetries( + context.Background(), s.WebhookID, + ) + + iAssertTerminallyFailed( + t, s, deliveryID, database.TargetTypeDatabase, + ) +} + +func TestRecoverSingleRetry_UnknownTargetType( + t *testing.T, +) { + t.Parallel() + + s := newISetup(t) + + iCreateWebhook( + t, s.MainDB, s.WebhookID, "unknown-type", + ) + + unknown := database.TargetType("not-a-target-type") + + deliveryID := iSeedRetryingWithType(t, s, unknown) + + s.Engine.ExportRecoverWebhookDeliveries( + context.Background(), s.WebhookID, + ) + + iAssertTerminallyFailed(t, s, deliveryID, unknown) +} + +func TestSweepSingleRetry_UnknownTargetType( + t *testing.T, +) { + t.Parallel() + + s := newISetup(t) + + iCreateWebhook( + t, s.MainDB, s.WebhookID, "unknown-type-sweep", + ) + + unknown := database.TargetType("not-a-target-type") + + deliveryID := iSeedRetryingWithType(t, s, unknown) + + s.Engine.ExportSweepWebhookRetries( + context.Background(), s.WebhookID, + ) + + iAssertTerminallyFailed(t, s, deliveryID, unknown) } // iSeedFailedResult creates a failed delivery result. diff --git a/internal/delivery/engine_lifecycle_test.go b/internal/delivery/engine_lifecycle_test.go new file mode 100644 index 0000000..40b1fde --- /dev/null +++ b/internal/delivery/engine_lifecycle_test.go @@ -0,0 +1,199 @@ +package delivery_test + +import ( + "context" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + "go.uber.org/fx" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/delivery" +) + +const ( + // hookStopTimeout bounds how long a lifecycle test waits for + // the engine's OnStop hook to return before declaring the + // shutdown hung. + hookStopTimeout = 10 * time.Second + + // hookSettleDelay is how long startEngineViaHook waits after + // OnStart before the caller may enqueue work. A worker pool + // wrongly rooted in the already-done hook context has nothing + // but ctx.Done() ready in its select, so it is deterministically + // gone by the end of this window. Without the wait, Notify would + // race the pool's very first select, in which a ready ctx.Done() + // and a ready deliveryCh are chosen between at random and a + // doomed pool still delivers. + hookSettleDelay = 250 * time.Millisecond +) + +// recordingLifecycle is a minimal fx.Lifecycle that records the +// hooks a component registers, so a test can invoke the real +// OnStart/OnStop functions with a context of its choosing. +type recordingLifecycle struct { + hooks []fx.Hook +} + +func (l *recordingLifecycle) Append(h fx.Hook) { + l.hooks = append(l.hooks, h) +} + +// startEngineViaHook drives the genuine fx hooks the application +// registers for the engine, handing OnStart a context that is +// already done, and returns only once a pool that inherited that +// context would have exited. It returns the recorded lifecycle so +// the caller can drive OnStop too. +// +// Callers must not seed pending or retrying deliveries before +// calling this: restart recovery enqueues those during startup, +// which would put work in the queue while the pool is still +// racing its first select. +func startEngineViaHook( + t *testing.T, eng *delivery.Engine, +) *recordingLifecycle { + t.Helper() + + lc := &recordingLifecycle{} + eng.ExportRegisterHooks(lc) + require.Len(t, lc.hooks, 1) + + // fx hands OnStart a context carrying the application start + // timeout, and cancels it when the start phase ends. An + // already-cancelled context is that same defect taken to its + // limit, and unlike a plain context.Background() it actually + // distinguishes a correctly rooted loop from a broken one. + hookCtx, cancel := context.WithCancel(context.Background()) + cancel() + + require.NoError(t, lc.hooks[0].OnStart(hookCtx)) + + time.Sleep(hookSettleDelay) + + return lc +} + +// seedLogTask seeds a pending delivery for a log target and +// returns its ID together with the task that drives it. The log +// target needs no network, so a delivery completing proves only +// that a worker picked the task up. +func seedLogTask( + t *testing.T, s iSetup, +) (string, delivery.Task) { + t.Helper() + + event := iSeedEvent( + t, s.WebhookDB, s.WebhookID, + `{"lifecycle":"hook-context"}`, + ) + targetID := uuid.New().String() + + d := iSeedDelivery( + t, s.WebhookDB, event.ID, targetID, + database.DeliveryStatusPending, + ) + + bodyStr := event.Body + task := iTask( + d, event, s.WebhookID, targetID, + "hook-context-test", "", 0, 1, &bodyStr, + ) + task.TargetType = database.TargetTypeLog + + return d.ID, task +} + +// TestEngine_WorkersOutliveStartHookContext is the regression +// test for a delivery engine that stopped delivering roughly +// fifteen seconds after boot. fx calls OnStart with a context +// carrying the application's start timeout (15s by default) and +// cancels it when the start phase ends, so a worker pool rooted +// in it exits shortly after startup: the process keeps accepting +// and persisting events while nothing at all forwards them. +// +// Driving OnStart with an already-cancelled context is that +// defect taken to its limit. A pool that inherits the hook +// context is gone before the task is even enqueued; a correctly +// rooted pool keeps working for as long as the process lives. +func TestEngine_WorkersOutliveStartHookContext(t *testing.T) { + t.Parallel() + + s := newISetup(t) + + lc := startEngineViaHook(t, s.Engine) + t.Cleanup(func() { + _ = lc.hooks[0].OnStop(context.Background()) + }) + + // Seeded only after the pool has settled, so restart recovery + // cannot enqueue it during startup. + deliveryID, task := seedLogTask(t, s) + + s.Engine.Notify([]delivery.Task{task}) + + iWaitForDelivered(t, s.WebhookDB, deliveryID) +} + +// TestEngine_StopHookStopsWorkers proves the fix did not trade a +// startup bug for a shutdown hang: now that the worker pool no +// longer observes the start hook's cancellation, OnStop is the +// only thing that can stop it, and it must both return promptly +// and actually leave the pool drained. +func TestEngine_StopHookStopsWorkers(t *testing.T) { + t.Parallel() + + s := newISetup(t) + + lc := startEngineViaHook(t, s.Engine) + + // Let the pool prove it is running before stopping it, so a + // fast OnStop cannot pass by stopping something already dead. + firstID, firstTask := seedLogTask(t, s) + s.Engine.Notify([]delivery.Task{firstTask}) + iWaitForDelivered(t, s.WebhookDB, firstID) + + var stopErr error + + stopped := make(chan struct{}) + + go func() { + defer close(stopped) + + // stop blocks on the workers' WaitGroup, so returning at + // all proves every goroutine observed the cancellation. + stopErr = lc.hooks[0].OnStop(context.Background()) + }() + + select { + case <-stopped: + case <-time.After(hookStopTimeout): + t.Fatal( + "OnStop did not return: the delivery engine's " + + "WaitGroup is still waiting on a goroutine that " + + "never observed cancellation", + ) + } + + require.NoError(t, stopErr) + + // With every worker gone, a freshly notified task must sit + // untouched in the queue rather than being delivered. + secondID, secondTask := seedLogTask(t, s) + s.Engine.Notify([]delivery.Task{secondTask}) + + time.Sleep(200 * time.Millisecond) + + var after database.Delivery + + require.NoError( + t, + s.WebhookDB.First(&after, "id = ?", secondID).Error, + ) + require.Equal( + t, + database.DeliveryStatusPending, + after.Status, + "a stopped engine must not deliver anything", + ) +} diff --git a/internal/delivery/export_test.go b/internal/delivery/export_test.go index 739eb03..326cbc1 100644 --- a/internal/delivery/export_test.go +++ b/internal/delivery/export_test.go @@ -7,10 +7,17 @@ import ( "net/http" "time" + "go.uber.org/fx" "gorm.io/gorm" "sneak.berlin/go/webhooker/internal/database" ) +// ErrExportArchiveWriterEvicted exposes the sentinel returned by +// an evicted archive writer. It carries the Err prefix rather +// than this file's usual Export one because it is a sentinel +// error. +var ErrExportArchiveWriterEvicted = errArchiveWriterEvicted + // Exported constants for test access. const ( ExportDeliveryChannelSize = deliveryChannelSize @@ -188,9 +195,24 @@ func (e *Engine) ExportRecoverInFlight( e.recoverInFlight(ctx) } +// ExportSweepWebhookRetries exposes sweepWebhookRetries. +func (e *Engine) ExportSweepWebhookRetries( + ctx context.Context, webhookID string, +) { + e.sweepWebhookRetries(ctx, webhookID) +} + // ExportStart exposes start for testing. -func (e *Engine) ExportStart(ctx context.Context) { - e.start(ctx) +func (e *Engine) ExportStart() { + e.start() +} + +// ExportRegisterHooks registers the engine's real fx lifecycle +// hooks on a lifecycle supplied by a test, so a test can drive +// the exact OnStart/OnStop functions the application runs and +// hand OnStart the kind of context fx actually supplies. +func (e *Engine) ExportRegisterHooks(lc fx.Lifecycle) { + e.registerHooks(lc) } // ExportStop exposes stop for testing. @@ -328,6 +350,183 @@ func (e *ExportArchiveWriter) DB() *gorm.DB { return e.w.db } +// Path returns the archive file the writer owns. +func (e *ExportArchiveWriter) Path() string { + return e.w.path +} + +// OpenExisting opens the archive without permitting creation, +// the way the idle sweep does. +func (e *ExportArchiveWriter) OpenExisting( + expiry time.Duration, +) error { + return e.w.openMode(archiveModeExisting, expiry) +} + +// SweepExpired runs an idle sweep of the archive. +func (e *ExportArchiveWriter) SweepExpired( + expiry time.Duration, +) error { + return e.w.sweepExpired(expiry) +} + +// Evict marks the writer evicted and closes its handle, exactly +// as leaving the registry does. +func (e *ExportArchiveWriter) Evict() { + e.w.evict() +} + +// HandleOpen reports whether the writer currently holds an open +// archive handle. +func (e *ExportArchiveWriter) HandleOpen() bool { + e.w.mu.Lock() + defer e.w.mu.Unlock() + + return e.w.db != nil +} + +// Same reports whether both wrappers refer to the very same +// underlying archive writer, so a test can prove a registry entry +// is the writer it was handed rather than a replacement. +func (e *ExportArchiveWriter) Same( + other *ExportArchiveWriter, +) bool { + return other != nil && e.w == other.w +} + +// ExportArchiveWriterFor returns the archive writer the registry +// currently caches for a webhook, or nil when none is cached. It +// never creates one, so a test can hold a reference to the very +// writer an eviction is about to detach. +func (e *Engine) ExportArchiveWriterFor( + webhookID string, +) *ExportArchiveWriter { + e.dbTarget.mu.Lock() + defer e.dbTarget.mu.Unlock() + + w, ok := e.dbTarget.writers[webhookID] + if !ok { + return nil + } + + return &ExportArchiveWriter{w: w} +} + +// ExportHasArchiveWriter reports whether the database target +// currently caches an archive writer for a webhook. +func (e *Engine) ExportHasArchiveWriter( + webhookID string, +) bool { + e.dbTarget.mu.Lock() + defer e.dbTarget.mu.Unlock() + + _, ok := e.dbTarget.writers[webhookID] + + return ok +} + +// ExportArchiveHandleOpen reports whether the cached archive +// writer for a webhook holds an open database handle. It +// returns false when no writer is cached. +func (e *Engine) ExportArchiveHandleOpen( + webhookID string, +) bool { + e.dbTarget.mu.Lock() + w, ok := e.dbTarget.writers[webhookID] + e.dbTarget.mu.Unlock() + + if !ok { + return false + } + + w.mu.Lock() + defer w.mu.Unlock() + + return w.db != nil +} + +// ExportEnsureArchiveWriter creates (if needed) and returns the +// archive file path of the cached writer for a webhook, so a +// test can prime the registry the way a delivery would. +func (e *Engine) ExportEnsureArchiveWriter( + webhookID string, +) (string, error) { + w, err := e.dbTarget.writerFor(webhookID) + if err != nil { + return "", err + } + + return w.path, nil +} + +// ExportSweepWriterFor takes a webhook's registry writer exactly +// as the idle sweep does, reporting whether the sweep had to +// create the entry. It lets a test drive the registry through the +// sweep's own entry point instead of choreographing goroutines. +func (e *Engine) ExportSweepWriterFor( + webhookID string, +) (*ExportArchiveWriter, bool, error) { + w, created, err := e.dbTarget.sweepWriterFor(webhookID) + if err != nil { + return nil, false, err + } + + return &ExportArchiveWriter{w: w}, created, nil +} + +// ExportReleaseSweepWriter releases a sweep-created registry entry +// exactly as a finished sweep does. +func (e *Engine) ExportReleaseSweepWriter( + webhookID string, w *ExportArchiveWriter, +) { + e.dbTarget.releaseSweepWriter(webhookID, w.w) +} + +// NewTestArchiveSweeper builds an ArchiveSweeper backed by the +// given main database and engine, without the fx lifecycle. +// Intended for tests. +func NewTestArchiveSweeper( + db *database.Database, + eng *Engine, + log *slog.Logger, +) *ArchiveSweeper { + return &ArchiveSweeper{ + db: db, + eng: eng, + log: log, + interval: time.Hour, + } +} + +// ExportSweep runs a single archive sweep synchronously for +// tests. +func (s *ArchiveSweeper) ExportSweep(ctx context.Context) { + s.sweep(ctx) +} + +// ExportStart starts the sweeper's background loop for tests. +func (s *ArchiveSweeper) ExportStart() { + s.start() +} + +// ExportRegisterHooks registers the sweeper's real fx lifecycle +// hooks on a lifecycle supplied by a test, so a test can drive +// the exact OnStart/OnStop functions the application runs and +// hand OnStart the kind of context fx actually supplies. +func (s *ArchiveSweeper) ExportRegisterHooks(lc fx.Lifecycle) { + s.registerHooks(lc) +} + +// ExportStop stops the sweeper's background loop for tests. +func (s *ArchiveSweeper) ExportStop() { + s.stop() +} + +// ExportSetInterval overrides the sweep interval for tests. +func (s *ArchiveSweeper) ExportSetInterval(d time.Duration) { + s.interval = d +} + // ExportParseArchiveExpiry exposes parseArchiveExpiry. func ExportParseArchiveExpiry( configJSON string, diff --git a/internal/delivery/ssrf.go b/internal/delivery/ssrf.go index be23746..fb6bacc 100644 --- a/internal/delivery/ssrf.go +++ b/internal/delivery/ssrf.go @@ -92,7 +92,12 @@ func ValidateTargetURL( ) error { parsed, err := url.Parse(targetURL) if err != nil { - return fmt.Errorf("invalid URL: %w", err) + // url.Parse embeds the whole URL in its error, and + // this one is logged and shown; mask it. Every other + // branch below reports only the hostname. + return fmt.Errorf( + "invalid URL: %w", maskURLError(err), + ) } err = validateScheme(parsed.Scheme) diff --git a/internal/delivery/target.go b/internal/delivery/target.go index 264ce2f..9e131b2 100644 --- a/internal/delivery/target.go +++ b/internal/delivery/target.go @@ -90,12 +90,15 @@ func (e *Engine) initTargets(client *http.Client) { client: client, } + dbT := &databaseTarget{eng: e} + e.httpTarget = httpT + e.dbTarget = dbT e.targets = map[database.TargetType]Target{ database.TargetTypeHTTP: httpT, database.TargetTypeSlack: slackT, - database.TargetTypeDatabase: &databaseTarget{eng: e}, + database.TargetTypeDatabase: dbT, database.TargetTypeLog: &logTarget{eng: e}, } } diff --git a/internal/delivery/target_config_view.go b/internal/delivery/target_config_view.go new file mode 100644 index 0000000..8beb182 --- /dev/null +++ b/internal/delivery/target_config_view.go @@ -0,0 +1,202 @@ +package delivery + +import ( + "encoding/json" + "fmt" + "strconv" + + "sneak.berlin/go/webhooker/internal/database" +) + +// configUnavailable is what a target's configuration renders +// as when it is absent, of an unknown type, or does not +// parse. The stored blob is never shown as a fallback: it can +// hold a credential (a Slack incoming webhook URL is a bearer +// token) and a UI that prints it leaks that credential into +// browser history, screenshots and screen shares. +const configUnavailable = "(unavailable)" + +// ConfigField is one labelled, display-safe value derived +// from a target's stored configuration. +type ConfigField struct { + Label string + Value string +} + +// TargetView is the display-safe projection of a target for +// the UI. It deliberately has no raw configuration field, so +// no template — present or future — can render the stored +// blob. +type TargetView struct { + ID string + Name string + Type database.TargetType + Active bool + Config []ConfigField +} + +// NewTargetViews projects targets for rendering, replacing +// each stored configuration blob with named, display-safe +// fields. +func NewTargetViews( + targets []database.Target, +) []TargetView { + views := make([]TargetView, 0, len(targets)) + + for i := range targets { + t := &targets[i] + + views = append(views, TargetView{ + ID: t.ID, + Name: t.Name, + Type: t.Type, + Active: t.Active, + Config: targetConfigFields(t), + }) + } + + return views +} + +// targetConfigFields returns the display-safe fields for a +// target's configuration. Anything it cannot parse becomes +// the neutral placeholder. +func targetConfigFields( + t *database.Target, +) []ConfigField { + switch t.Type { + case database.TargetTypeSlack: + return slackConfigFields(t.Config) + case database.TargetTypeHTTP: + return httpConfigFields(t) + case database.TargetTypeDatabase: + return databaseConfigFields(t.Config) + case database.TargetTypeLog: + // The log target takes no configuration. + return nil + default: + return unavailableConfigFields() + } +} + +// unavailableConfigFields is the neutral placeholder shown +// for a configuration that could not be presented. +func unavailableConfigFields() []ConfigField { + return []ConfigField{{ + Label: "Configuration", + Value: configUnavailable, + }} +} + +// slackConfigFields describes a Slack target. Only the masked +// webhook URL is shown; the full URL is the credential. +func slackConfigFields(configJSON string) []ConfigField { + cfg, err := parseSlackConfig(configJSON) + if err != nil { + return unavailableConfigFields() + } + + return []ConfigField{{ + Label: "Webhook URL", + Value: cfg.MaskedWebhookURL(), + }} +} + +// httpConfigFields describes an HTTP target: its destination +// and its retry settings. Header values are not shown — they +// routinely carry authorization tokens — only how many are +// configured. +func httpConfigFields(t *database.Target) []ConfigField { + cfg, err := parseHTTPConfig(t.Config) + if err != nil { + return unavailableConfigFields() + } + + fields := []ConfigField{{ + Label: "Destination URL", + Value: cfg.URL, + }} + + if cfg.Timeout > 0 { + fields = append(fields, ConfigField{ + Label: "Timeout", + Value: strconv.Itoa(cfg.Timeout) + "s", + }) + } + + if len(cfg.Headers) > 0 { + fields = append(fields, ConfigField{ + Label: "Headers", + Value: fmt.Sprintf( + "%d configured", len(cfg.Headers), + ), + }) + } + + return append(fields, retryFields(t)...) +} + +// retryFields describes a target's retry settings, which live +// on the target row rather than in its configuration blob. +func retryFields(t *database.Target) []ConfigField { + retries := strconv.Itoa(t.MaxRetries) + if t.MaxRetries == 0 { + retries += " (fire-and-forget)" + } + + fields := []ConfigField{{ + Label: "Max Retries", + Value: retries, + }} + + if t.MaxQueueSize > 0 { + fields = append(fields, ConfigField{ + Label: "Max Queue Size", + Value: strconv.Itoa(t.MaxQueueSize), + }) + } + + return fields +} + +// databaseConfigFields describes an archive target. Its +// configuration is optional, and an absent or empty expiry +// means the archive is kept forever. An expiry that is set +// but not a valid duration is reported as unavailable rather +// than echoed back. +func databaseConfigFields(configJSON string) []ConfigField { + expiry := archiveExpiryNever + + if configJSON != "" { + var cfg databaseTargetConfig + + err := json.Unmarshal([]byte(configJSON), &cfg) + if err != nil { + return unavailableConfigFields() + } + + if cfg.Expiry != "" { + if ValidateArchiveExpiry(cfg.Expiry) != nil { + return unavailableConfigFields() + } + + expiry = cfg.Expiry + } + } + + return []ConfigField{{ + Label: "Archive Expiry", + Value: expiry, + }} +} + +// MaskedWebhookURL returns the Slack webhook URL reduced to +// its scheme and host, with the path, query and any userinfo +// elided. The path segments are the credential, so none of +// them is shown: the field accepts an arbitrary URL, so no +// segment can be assumed non-secret. A URL that does not +// parse into a scheme and host yields the neutral +// placeholder, never the raw string. +func (c *SlackTargetConfig) MaskedWebhookURL() string { + return MaskURL(c.WebhookURL) +} diff --git a/internal/delivery/target_config_view_test.go b/internal/delivery/target_config_view_test.go new file mode 100644 index 0000000..0d5d910 --- /dev/null +++ b/internal/delivery/target_config_view_test.go @@ -0,0 +1,299 @@ +package delivery_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/delivery" +) + +const ( + // slackSecretPath is the credential-bearing part of a + // Slack incoming webhook URL: everything after the host. + slackSecretPath = "/services/T00000000/B00000000/" + + "XXXXXXXXXXXXXXXXXXXXXXXX" + slackWebhookURL = "https://hooks.slack.com" + + slackSecretPath + + viewExampleOrigin = "https://example.com" + viewExampleHook = viewExampleOrigin + "/hook" + viewUnavailable = "(unavailable)" + viewExpiryNever = "never" +) + +func TestMaskedWebhookURL(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + url string + want string + }{ + "slack webhook": { + url: slackWebhookURL, + want: "https://hooks.slack.com/...", + }, + "query string dropped": { + url: viewExampleOrigin + "/a?token=secret", + want: viewExampleOrigin + "/...", + }, + // Fabricated userinfo in a test URL, not a real + // credential. + //nolint:gosec // G101 + "userinfo dropped": { + url: "https://user:pw@example.com/a/b", + want: viewExampleOrigin + "/...", + }, + "no path": { + url: viewExampleOrigin, + want: viewExampleOrigin, + }, + "root path": { + url: viewExampleOrigin + "/", + want: viewExampleOrigin, + }, + "not a url": { + url: "definitely not a url", + want: viewUnavailable, + }, + "empty": { + url: "", + want: viewUnavailable, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + cfg := &delivery.SlackTargetConfig{ + WebhookURL: tc.url, + } + + assert.Equal( + t, tc.want, cfg.MaskedWebhookURL(), + ) + }) + } +} + +// TestMaskedWebhookURL_NeverLeaksPath is the direct +// expression of the rule: whatever the input, the masked +// value never contains a path segment of it. +func TestMaskedWebhookURL_NeverLeaksPath(t *testing.T) { + t.Parallel() + + cfg := &delivery.SlackTargetConfig{ + WebhookURL: slackWebhookURL, + } + + masked := cfg.MaskedWebhookURL() + + assert.NotContains(t, masked, "T00000000") + assert.NotContains(t, masked, "B00000000") + assert.NotContains( + t, masked, "XXXXXXXXXXXXXXXXXXXXXXXX", + ) + assert.NotContains(t, masked, slackSecretPath) +} + +// fieldMap turns a view's config fields into a lookup so +// assertions read by label. +func fieldMap(fields []delivery.ConfigField) map[string]string { + out := make(map[string]string, len(fields)) + for _, f := range fields { + out[f.Label] = f.Value + } + + return out +} + +// viewFor projects a single target and returns its view. +func viewFor( + t *testing.T, + target database.Target, +) delivery.TargetView { + t.Helper() + + views := delivery.NewTargetViews( + []database.Target{target}, + ) + require.Len(t, views, 1) + + return views[0] +} + +func TestNewTargetViews_Slack(t *testing.T) { + t.Parallel() + + view := viewFor(t, database.Target{ + Name: "slack-target", + Type: database.TargetTypeSlack, + Active: true, + Config: `{"webhookUrl":"` + + slackWebhookURL + `"}`, + }) + + assert.Equal(t, "slack-target", view.Name) + assert.Equal( + t, + map[string]string{ + "Webhook URL": "https://hooks.slack.com/...", + }, + fieldMap(view.Config), + ) +} + +func TestNewTargetViews_HTTP(t *testing.T) { + t.Parallel() + + view := viewFor(t, database.Target{ + Type: database.TargetTypeHTTP, + Config: `{"url":"` + viewExampleHook + `",` + + `"timeout":30,` + + `"headers":{"Authorization":"Bearer sekrit"}}`, + MaxRetries: 5, + MaxQueueSize: 100, + }) + + fields := fieldMap(view.Config) + + assert.Equal( + t, + map[string]string{ + "Destination URL": viewExampleHook, + "Timeout": "30s", + "Headers": "1 configured", + "Max Retries": "5", + "Max Queue Size": "100", + }, + fields, + ) + + // Header values can be credentials and are never shown. + for _, v := range fields { + assert.NotContains(t, v, "sekrit") + } +} + +func TestNewTargetViews_HTTPFireAndForget(t *testing.T) { + t.Parallel() + + view := viewFor(t, database.Target{ + Type: database.TargetTypeHTTP, + Config: `{"url":"` + viewExampleHook + `"}`, + }) + + assert.Equal( + t, + map[string]string{ + "Destination URL": viewExampleHook, + "Max Retries": "0 (fire-and-forget)", + }, + fieldMap(view.Config), + ) +} + +func TestNewTargetViews_Database(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + config string + want string + }{ + "empty config": {config: "", want: viewExpiryNever}, + "empty expiry": {config: `{}`, want: viewExpiryNever}, + "explicit": { + config: `{"expiry":"720h"}`, + want: "720h", + }, + "never literal": { + config: `{"expiry":"` + viewExpiryNever + `"}`, + want: viewExpiryNever, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + view := viewFor(t, database.Target{ + Type: database.TargetTypeDatabase, + Config: tc.config, + }) + + assert.Equal( + t, + map[string]string{"Archive Expiry": tc.want}, + fieldMap(view.Config), + ) + }) + } +} + +func TestNewTargetViews_Log(t *testing.T) { + t.Parallel() + + view := viewFor(t, database.Target{ + Type: database.TargetTypeLog, + Config: "", + }) + + assert.Empty(t, view.Config) +} + +// TestNewTargetViews_Unpresentable proves that no config the +// view cannot present falls back to the stored blob. +func TestNewTargetViews_Unpresentable(t *testing.T) { + t.Parallel() + + const blob = `{"webhookUrl":"https://hooks.slack.com` + + slackSecretPath + `"` + + tests := map[string]database.Target{ + "unknown target type": { + Type: database.TargetType("carrier-pigeon"), + Config: blob, + }, + "unparseable json": { + Type: database.TargetTypeSlack, + Config: blob, + }, + "empty slack config": { + Type: database.TargetTypeSlack, + }, + "slack config without url": { + Type: database.TargetTypeSlack, + Config: `{}`, + }, + "unparseable http json": { + Type: database.TargetTypeHTTP, + Config: `{"url":`, + }, + "unparseable archive json": { + Type: database.TargetTypeDatabase, + Config: `{"expiry":`, + }, + "invalid archive expiry": { + Type: database.TargetTypeDatabase, + Config: `{"expiry":"a fortnight"}`, + }, + } + + for name, target := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + view := viewFor(t, target) + + assert.Equal( + t, + map[string]string{ + "Configuration": viewUnavailable, + }, + fieldMap(view.Config), + ) + }) + } +} diff --git a/internal/delivery/target_database.go b/internal/delivery/target_database.go index 0443aaa..db29520 100644 --- a/internal/delivery/target_database.go +++ b/internal/delivery/target_database.go @@ -5,6 +5,7 @@ import ( "fmt" "path/filepath" "sync" + "time" "gorm.io/gorm" "sneak.berlin/go/webhooker/internal/database" @@ -111,15 +112,11 @@ func (t *databaseTarget) archive(d *database.Delivery) error { func (t *databaseTarget) writerFor( webhookID string, ) (*archiveWriter, error) { - if t.eng.dbManager == nil { - return nil, errArchiveNoDataDir + path, err := t.archivePath(webhookID) + if err != nil { + return nil, err } - dir := filepath.Dir(t.eng.dbManager.DBPath(webhookID)) - path := filepath.Join( - dir, fmt.Sprintf("archive-%s.db", webhookID), - ) - t.mu.Lock() defer t.mu.Unlock() @@ -133,5 +130,166 @@ func (t *databaseTarget) writerFor( t.writers[webhookID] = w } + // A delivery claims the entry: even if the idle sweep created + // it moments ago, it now belongs to the registry proper and + // the sweep must leave it in place when it finishes. + w.sweepOwned = false + return w, nil } + +// sweepWriterFor returns the archive writer the idle sweep should +// prune a webhook through, together with whether the sweep itself +// created the registry entry. +// +// The sweep must route its prune through the registered writer so +// the writer's mutex orders it against concurrent writes, but it +// must never leave a registry entry behind: a sweep that ran +// concurrently with the webhook's deletion would otherwise +// re-create an entry that nothing will ever evict again, which is +// exactly the leak eviction exists to prevent. An entry the sweep +// creates is therefore marked sweep-owned and handed back to +// releaseSweepWriter when the sweep is done. +func (t *databaseTarget) sweepWriterFor( + webhookID string, +) (*archiveWriter, bool, error) { + path, err := t.archivePath(webhookID) + if err != nil { + return nil, false, err + } + + t.mu.Lock() + defer t.mu.Unlock() + + if t.writers == nil { + t.writers = make(map[string]*archiveWriter) + } + + w, ok := t.writers[webhookID] + if ok { + return w, false, nil + } + + w = newArchiveWriter(path, t.eng.log) + w.sweepOwned = true + t.writers[webhookID] = w + + return w, true, nil +} + +// releaseSweepWriter drops a registry entry that the idle sweep +// created, so a sweep leaves the registry exactly as it found it. +// +// The entry is removed only if it is still the very writer the +// sweep installed and no delivery has claimed it in the meantime +// (writerFor clears sweepOwned when it hands a writer to the +// write path). Both conditions are evaluated under the registry +// lock, so an eviction that raced the sweep — which removes the +// entry outright — simply finds nothing left to do here, and a +// delivery that adopted the writer keeps a registered, evictable +// one. +func (t *databaseTarget) releaseSweepWriter( + webhookID string, w *archiveWriter, +) { + t.mu.Lock() + defer t.mu.Unlock() + + cur, ok := t.writers[webhookID] + if !ok || cur != w || !cur.sweepOwned { + return + } + + delete(t.writers, webhookID) +} + +// archivePath returns the archive file path for a webhook: it +// lives beside the per-webhook event database in the data +// directory. It does not touch the filesystem. +func (t *databaseTarget) archivePath( + webhookID string, +) (string, error) { + if t.eng.dbManager == nil { + return "", errArchiveNoDataDir + } + + dir := filepath.Dir(t.eng.dbManager.DBPath(webhookID)) + + return filepath.Join( + dir, fmt.Sprintf("archive-%s.db", webhookID), + ), nil +} + +// evict drops a webhook's archive writer from the registry and +// closes its handle, so a deleted webhook does not leave a +// writer (and an open archive handle within its debounce +// window) alive for the process lifetime. +// +// The map entry is removed under the registry lock, which is +// then released before the handle is closed under the writer's +// own lock: that ordering keeps the registry available to other +// webhooks while an in-flight write on this one drains, and +// closing under the writer's lock means eviction can never race +// a write. +// +// Eviction is idempotent and silent for a webhook with no +// writer, which is the common case: a webhook with no database +// target never creates one. It never deletes the archive file. +func (t *databaseTarget) evict(webhookID string) { + t.mu.Lock() + + w, ok := t.writers[webhookID] + if ok { + delete(t.writers, webhookID) + } + + t.mu.Unlock() + + if !ok { + return + } + + w.evict() + + t.eng.log.Info( + "evicted archive writer", + "webhook_id", webhookID, + "path", w.path, + ) +} + +// sweepWebhook prunes one webhook's archive of rows older than +// expiry, without requiring a write. It returns nil (nothing to +// do) when the archive file does not exist, so a sweep never +// creates an archive for a webhook that has a database target +// but has never received an event. +// +// It also never leaves a registry entry behind: an entry it had +// to create to reach the writer's mutex is released again once +// the prune is done, so a sweep racing a webhook deletion cannot +// resurrect the writer the eviction just dropped. +func (t *databaseTarget) sweepWebhook( + webhookID string, expiry time.Duration, +) error { + path, err := t.archivePath(webhookID) + if err != nil { + return err + } + + // Check before taking a writer at all: a webhook whose + // archive has never been created gets no writer, no handle, + // and no file. + if !fileExists(path) { + return nil + } + + w, created, err := t.sweepWriterFor(webhookID) + if err != nil { + return err + } + + if created { + defer t.releaseSweepWriter(webhookID, w) + } + + return w.sweepExpired(expiry) +} diff --git a/internal/delivery/target_database_archive.go b/internal/delivery/target_database_archive.go index 547a0af..f2ca64b 100644 --- a/internal/delivery/target_database_archive.go +++ b/internal/delivery/target_database_archive.go @@ -24,6 +24,20 @@ const archiveExpiryNever = "never" // offline archiving, but never more than once per this window. const archiveReopenDebounce = time.Second +const ( + // archiveModeCreate is the SQLite URI mode used by the write + // path: open the archive file, creating it if missing, so a + // first write (or a write after the operator moved the file + // away) recreates it. + archiveModeCreate = "rwc" + + // archiveModeExisting is the SQLite URI mode used by the idle + // sweep: open read-write but never create. A sweep must never + // conjure an empty archive file for a webhook that has a + // database target but has never received an event. + archiveModeExisting = "rw" +) + var ( // errArchiveMissingWebhookID is returned when an event to // archive has no webhook id to key its archive file on. @@ -44,6 +58,15 @@ var ( errArchiveExpiryNotPositive = errors.New( "expiry must be a positive duration or \"never\"", ) + + // errArchiveWriterEvicted is returned when a writer that has + // been evicted (its webhook was deleted, or its last database + // target was removed) is used again. An evicted writer is no + // longer in the registry, so reopening its file would leak a + // handle nothing owns. + errArchiveWriterEvicted = errors.New( + "archive writer has been evicted", + ) ) // databaseTargetConfig is the optional per-target JSON config @@ -161,6 +184,25 @@ type archiveWriter struct { db *gorm.DB lastReopen time.Time reopens int + + // evicted marks a writer that has been removed from the + // per-webhook registry. Its handle is closed and it must + // never open the file again: nothing holds it any more, so a + // reopen would leak the handle for the process lifetime. + evicted bool + + // sweepOwned marks a registry entry that the idle sweep + // created because no writer was cached for the webhook. The + // sweep removes such an entry again when it is done, so a + // sweep can never leave — or resurrect — a registry entry + // for a webhook that has been deleted. A delivery that adopts + // the writer clears the flag, handing the entry to the + // registry proper. + // + // Unlike every other field here it is guarded by + // databaseTarget.mu, not by this writer's mu: it describes the + // registry entry rather than the file. + sweepOwned bool } // newArchiveWriter builds an archiveWriter for a file path with @@ -185,6 +227,12 @@ func (w *archiveWriter) write( w.mu.Lock() defer w.mu.Unlock() + if w.evicted { + return fmt.Errorf( + "%w: %s", errArchiveWriterEvicted, w.path, + ) + } + if w.db == nil || !fileExists(w.path) { err := w.reopen(expiry) if err != nil { @@ -212,7 +260,19 @@ func (w *archiveWriter) write( // its schema, records the reopen time, and prunes expired rows // when expiry is positive. func (w *archiveWriter) open(expiry time.Duration) error { - dbURL := fmt.Sprintf("file:%s?mode=rwc", w.path) + return w.openMode(archiveModeCreate, expiry) +} + +// openMode opens the archive file with the given SQLite URI +// mode, migrates its schema, records the reopen time, and +// prunes expired rows when expiry is positive. The write path +// passes archiveModeCreate so a missing file is recreated; the +// idle sweep passes archiveModeExisting so a missing file is an +// error rather than a newly conjured empty archive. +func (w *archiveWriter) openMode( + mode string, expiry time.Duration, +) error { + dbURL := fmt.Sprintf("file:%s?mode=%s", w.path, mode) sqlDB, err := sql.Open("sqlite", dbURL) if err != nil { @@ -275,11 +335,70 @@ func (w *archiveWriter) close() { w.db = nil } +// sweepExpired prunes an archive that may have gone idle, with +// no write to trigger the usual on-reopen prune. It takes the +// writer's own mutex for the whole operation, so a sweep is +// ordered against concurrent writes rather than reaching around +// them to the file. +// +// It never creates the archive file: a missing file is skipped, +// and the reopen uses archiveModeExisting so SQLite itself +// refuses to create one if the file disappears between the +// check and the open. +// +// The archive is left CLOSED afterwards. An idle archive holding +// no handle is what keeps the operator's move-the-file-away +// workflow working; the next write reopens (and recreates) the +// file as it always has. +func (w *archiveWriter) sweepExpired(expiry time.Duration) error { + w.mu.Lock() + defer w.mu.Unlock() + + if w.evicted { + return fmt.Errorf( + "%w: %s", errArchiveWriterEvicted, w.path, + ) + } + + if !fileExists(w.path) { + return nil + } + + // Drop any live handle first so the prune runs against a + // freshly opened file, matching the write path's semantics. + w.close() + + err := w.openMode(archiveModeExisting, expiry) + if err != nil { + return err + } + + w.close() + + return nil +} + +// evict closes the writer's handle and marks it unusable. It is +// called when the writer leaves the registry, either because the +// webhook was deleted or because its last database target was +// removed. The archive FILE is deliberately left on disk: it is +// long-term storage an operator may still want. +func (w *archiveWriter) evict() { + w.mu.Lock() + defer w.mu.Unlock() + + w.evicted = true + + w.close() +} + // prune deletes archived rows older than expiry, measured from -// each row's archived time. It runs on every (re)open, and -// because the file is reopened after writes this keeps the -// archive swept without a separate background sweeper. Failures -// are logged, not fatal: a prune error must not stop archiving. +// each row's archived time. It runs on every (re)open, so a +// steadily written archive is swept by its own write traffic. An +// archive that goes idle receives no further reopens, which is +// why ArchiveSweeper exists to drive sweepExpired on a timer. +// Failures are logged, not fatal: a prune error must not stop +// archiving. func (w *archiveWriter) prune(expiry time.Duration) { cutoff := time.Now().Add(-expiry) diff --git a/internal/delivery/target_database_evict_test.go b/internal/delivery/target_database_evict_test.go new file mode 100644 index 0000000..14e7945 --- /dev/null +++ b/internal/delivery/target_database_evict_test.go @@ -0,0 +1,363 @@ +package delivery_test + +import ( + "errors" + "fmt" + "net/http" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/delivery" +) + +// evictTestEngine builds an engine backed by a temporary data +// directory and returns it along with that directory. +func evictTestEngine(t *testing.T) (*delivery.Engine, string) { + t.Helper() + + dataDir := t.TempDir() + + eng := delivery.NewTestEngineWithDB( + nil, + database.NewTestWebhookDBManager(dataDir), + archiveTestLogger(), + &http.Client{Timeout: 5 * time.Second}, + 1, + ) + + return eng, dataDir +} + +// TestEvictWebhook_ClosesAndRemovesWriter proves that evicting +// a webhook drops its archive writer from the registry and +// closes the open archive handle, rather than leaving both +// alive for the process lifetime. +func TestEvictWebhook_ClosesAndRemovesWriter(t *testing.T) { + t.Parallel() + + eng, dataDir := evictTestEngine(t) + + webhookDB := testWebhookDB(t) + event := seedEvent(t, webhookDB, `{"archived":true}`) + d := seedDatabaseTargetDelivery(t, webhookDB, event, "") + + eng.ExportDeliverDatabase(webhookDB, d) + + webhookID := event.WebhookID + + require.True( + t, eng.ExportHasArchiveWriter(webhookID), + "a delivery should have cached an archive writer", + ) + require.True( + t, eng.ExportArchiveHandleOpen(webhookID), + "the writer should hold an open handle after a write", + ) + + eng.EvictWebhook(webhookID) + + assert.False( + t, eng.ExportHasArchiveWriter(webhookID), + "eviction should remove the registry entry", + ) + assert.False( + t, eng.ExportArchiveHandleOpen(webhookID), + "eviction should close the archive handle", + ) + + archivePath := filepath.Join( + dataDir, fmt.Sprintf("archive-%s.db", webhookID), + ) + assert.FileExists( + t, archivePath, + "eviction must not delete the archive file", + ) +} + +// TestEvictWebhook_UnknownWebhookIsNoOp proves eviction is safe +// for the common case of a webhook that never had a database +// target, and that repeating it does not panic. +func TestEvictWebhook_UnknownWebhookIsNoOp(t *testing.T) { + t.Parallel() + + eng, _ := evictTestEngine(t) + + assert.NotPanics(t, func() { + eng.EvictWebhook("no-such-webhook") + eng.EvictWebhook("no-such-webhook") + }) + + assert.False( + t, eng.ExportHasArchiveWriter("no-such-webhook"), + "eviction must not create a writer", + ) +} + +// evictTestRow builds an archive row for the eviction tests. +func evictTestRow(eventID string) delivery.ExportArchivedEvent { + return delivery.ExportArchivedEvent{ + EventID: eventID, + WebhookID: "wh-evict", + Method: http.MethodPost, + Body: `{"seeded":true}`, + } +} + +// TestEvictedWriter_WriteDoesNotReopenFile is the direct test of +// the evicted guard on the write path. A writer that has left +// the registry is held by nobody, so a handle it opened could +// never be closed again: it must refuse the write outright +// rather than recreate the archive behind the registry's back. +// +// The archive file is removed before the eviction, so an +// unguarded write is unmistakable — it recreates the file. +func TestEvictedWriter_WriteDoesNotReopenFile(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "archive-evicted.db") + + w := delivery.NewExportArchiveWriter( + path, archiveTestLogger(), 0, + ) + + require.NoError(t, w.Write(evictTestRow("ev-1"), 0)) + require.FileExists(t, path) + + // The operator moves the archive away for offline retention, + // which the write path would ordinarily undo on the next + // write by recreating the file. + require.NoError(t, os.Remove(path)) + + w.Evict() + + err := w.Write(evictTestRow("ev-2"), 0) + + require.ErrorIs( + t, err, delivery.ErrExportArchiveWriterEvicted, + "an evicted writer must refuse writes", + ) + assert.NoFileExists( + t, path, + "an evicted writer must not reopen (or recreate) the "+ + "archive file", + ) + assert.False( + t, w.HandleOpen(), + "an evicted writer must hold no handle", + ) +} + +// TestEvictedWriter_SweepDoesNotReopenFile is the same test for +// the sweep path: an idle sweep that reaches a writer already +// evicted underneath it must return the sentinel rather than +// reopen a file nothing owns. +func TestEvictedWriter_SweepDoesNotReopenFile(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "archive-evicted.db") + + w := delivery.NewExportArchiveWriter( + path, archiveTestLogger(), 0, + ) + + require.NoError(t, w.Write(evictTestRow("ev-1"), 0)) + require.FileExists(t, path) + + w.Evict() + + err := w.SweepExpired(time.Hour) + + require.ErrorIs( + t, err, delivery.ErrExportArchiveWriterEvicted, + "an evicted writer must refuse an idle sweep", + ) + assert.False( + t, w.HandleOpen(), + "a refused sweep must not leave a handle open", + ) +} + +// racingWrites drives a pack of goroutines writing to one +// archive writer until each is refused, so an eviction on the +// test goroutine has to take the writer's mutex away from writes +// that are already contending for it. +type racingWrites struct { + wg sync.WaitGroup + mu sync.Mutex + sawEvicted bool + otherErr error + started chan struct{} +} + +// racingWriteGoroutines is how many goroutines contend for the +// writer's mutex while the eviction lands. +const racingWriteGoroutines = 4 + +// startRacingWrites launches the writing goroutines. Each writes +// in a loop and stops at its first error, recording whether that +// error was the eviction sentinel. The deadline is a backstop +// against a hang, not a timing assumption: the first write after +// the eviction is refused. +func startRacingWrites( + w *delivery.ExportArchiveWriter, +) *racingWrites { + r := &racingWrites{ + started: make(chan struct{}, racingWriteGoroutines), + } + + deadline := time.Now().Add(10 * time.Second) + + r.wg.Add(racingWriteGoroutines) + + for i := range racingWriteGoroutines { + go func() { + defer r.wg.Done() + + first := true + + for time.Now().Before(deadline) { + err := w.Write( + evictTestRow(fmt.Sprintf("ev-%d", i)), 0, + ) + + if first { + r.started <- struct{}{} + + first = false + } + + if err == nil { + continue + } + + r.record(err) + + return + } + }() + } + + return r +} + +// record classifies the error that stopped one goroutine. +func (r *racingWrites) record(err error) { + r.mu.Lock() + defer r.mu.Unlock() + + if errors.Is(err, delivery.ErrExportArchiveWriterEvicted) { + r.sawEvicted = true + + return + } + + r.otherErr = err +} + +// awaitFirstWrite blocks until at least one write has run, so +// the eviction that follows is a genuine race. +func (r *racingWrites) awaitFirstWrite() { + <-r.started +} + +// wait joins the goroutines and reports whether any write was +// refused with the eviction sentinel, plus any unexpected error. +func (r *racingWrites) wait() (bool, error) { + r.wg.Wait() + + r.mu.Lock() + defer r.mu.Unlock() + + return r.sawEvicted, r.otherErr +} + +// TestEvictWebhook_RacingWriteDoesNotReopenHandle exercises the +// interleaving the evicted flag exists for: writes already +// contending for the writer's mutex when the eviction takes it. +// The write that wins the mutex after the eviction must abandon +// its work rather than reopen the archive, leaving the writer +// permanently handle-free. Run under -race. +func TestEvictWebhook_RacingWriteDoesNotReopenHandle( + t *testing.T, +) { + t.Parallel() + + eng, _ := evictTestEngine(t) + + webhookDB := testWebhookDB(t) + event := seedEvent(t, webhookDB, `{"archived":true}`) + d := seedDatabaseTargetDelivery(t, webhookDB, event, "") + + // Prime the registry so the test can hold the very writer the + // eviction is about to detach. + eng.ExportDeliverDatabase(webhookDB, d) + + w := eng.ExportArchiveWriterFor(event.WebhookID) + require.NotNil(t, w) + require.True(t, w.HandleOpen()) + + race := startRacingWrites(w) + + // Evict only once writes are genuinely in flight, so the + // eviction has to contend for the writer's mutex. + race.awaitFirstWrite() + + eng.EvictWebhook(event.WebhookID) + + sawEvicted, otherErr := race.wait() + + require.NoError(t, otherErr) + assert.True( + t, sawEvicted, + "a write after eviction must be refused", + ) + assert.False( + t, w.HandleOpen(), + "no write may reopen the archive once the writer has "+ + "been evicted", + ) + assert.False( + t, eng.ExportHasArchiveWriter(event.WebhookID), + "the registry entry must stay gone", + ) +} + +// TestEvictWebhook_LaterDeliveryRecreatesWriter proves eviction +// does not break archiving for a webhook that is still alive: a +// subsequent delivery gets a brand new writer from the registry. +// It says nothing about the evicted writer itself — that is what +// TestEvictedWriter_WriteDoesNotReopenFile covers. +func TestEvictWebhook_LaterDeliveryRecreatesWriter(t *testing.T) { + t.Parallel() + + eng, _ := evictTestEngine(t) + + webhookDB := testWebhookDB(t) + event := seedEvent(t, webhookDB, `{"archived":true}`) + d := seedDatabaseTargetDelivery(t, webhookDB, event, "") + + eng.ExportDeliverDatabase(webhookDB, d) + require.True( + t, eng.ExportHasArchiveWriter(event.WebhookID), + ) + + eng.EvictWebhook(event.WebhookID) + + // A fresh delivery for the same webhook gets a brand new + // writer from the registry, so archiving keeps working. + second := seedDatabaseTargetDelivery( + t, webhookDB, event, "", + ) + eng.ExportDeliverDatabase(webhookDB, second) + + assert.True( + t, eng.ExportHasArchiveWriter(event.WebhookID), + "a later delivery should recreate the writer", + ) +} diff --git a/internal/delivery/target_database_test.go b/internal/delivery/target_database_test.go index 3ee38ac..dc2e65c 100644 --- a/internal/delivery/target_database_test.go +++ b/internal/delivery/target_database_test.go @@ -50,6 +50,13 @@ func openArchiveDBForRead( return gdb } +// archiveFileSuffixes returns the archive file itself and the +// SQLite sidecars that accompany an open database. A test that +// asserts no archive was created has to check all of them. +func archiveFileSuffixes() []string { + return []string{"", "-wal", "-shm"} +} + // removeArchiveFiles simulates an operator moving the archive // away by deleting the SQLite file and its sidecar files. func removeArchiveFiles(t *testing.T, path string) { diff --git a/internal/delivery/target_http.go b/internal/delivery/target_http.go index 3ad6fb2..f39af45 100644 --- a/internal/delivery/target_http.go +++ b/internal/delivery/target_http.go @@ -363,7 +363,8 @@ func (t *httpTarget) doHTTPRequest( ) if reqErr != nil { return 0, "", 0, fmt.Errorf( - "creating request: %w", reqErr, + "creating request: %w", + maskURLError(reqErr), ) } @@ -492,8 +493,19 @@ func applyRequestHeaders( // executeHTTPRequest sends an HTTP request using the provided // client. URLs are validated by the config parsers and the // SSRF-safe transport before reaching here. +// +// Transport failures are masked here, at the single point +// where every target's request errors are born, because the +// caller stores them in DeliveryResult.Error: an unmasked +// *url.Error would write the target URL — the credential for +// a Slack incoming webhook — into the per-webhook database. func executeHTTPRequest( client *http.Client, req *http.Request, ) (*http.Response, error) { - return client.Do(req) //#nosec G704 -- validated URL, SSRF-safe transport + resp, err := client.Do(req) //#nosec G704 -- validated URL, SSRF-safe transport + if err != nil { + return nil, maskURLError(err) + } + + return resp, nil } diff --git a/internal/delivery/target_slack.go b/internal/delivery/target_slack.go index fdb95f6..5f98359 100644 --- a/internal/delivery/target_slack.go +++ b/internal/delivery/target_slack.go @@ -125,7 +125,7 @@ func (t *slackTarget) attempt( if err != nil { return attemptResult{ success: false, - errMsg: err.Error(), + errMsg: maskURLError(err).Error(), } } diff --git a/internal/delivery/url_mask.go b/internal/delivery/url_mask.go new file mode 100644 index 0000000..95821d2 --- /dev/null +++ b/internal/delivery/url_mask.go @@ -0,0 +1,61 @@ +package delivery + +import ( + "errors" + "net/url" +) + +// urlPathElision stands in for a URL's elided path. +const urlPathElision = "/..." + +// MaskURL renders a URL as scheme plus host with everything +// that can carry a secret removed. A delivery target URL is +// itself a credential — a Slack incoming webhook URL is a +// bearer token — so the path, query and userinfo are never +// reproduced, in a page, a log line or a stored error. A URL +// that does not parse into a scheme and host yields the +// neutral placeholder, never the raw string. +func MaskURL(raw string) string { + parsed, err := url.Parse(raw) + if err != nil || parsed.Scheme == "" || + parsed.Host == "" { + return configUnavailable + } + + masked := parsed.Scheme + "://" + parsed.Host + + if parsed.Path != "" && parsed.Path != "/" { + masked += urlPathElision + } + + return masked +} + +// maskURLError strips the credential from an error raised +// against a request URL. The net/http and net/url packages +// embed the full request URL in every *url.Error they return, +// so an unmodified transport error persisted into +// DeliveryResult.Error writes the credential to disk. +// +// The masked error keeps the operation and the wrapped cause, +// so a DNS failure still reads differently from a refused +// connection, a TLS handshake failure or a timeout, and Is, +// As, Timeout and Temporary keep working on it. Only the +// path, query and userinfo of the URL are dropped. Errors +// that carry no URL are returned unchanged. +// +// Call it where the error is raised, before any wrapping: it +// replaces the *url.Error itself, so any context wrapped +// around it first would be discarded. +func maskURLError(err error) error { + var urlErr *url.Error + if !errors.As(err, &urlErr) { + return err + } + + return &url.Error{ + Op: urlErr.Op, + URL: MaskURL(urlErr.URL), + Err: urlErr.Err, + } +} diff --git a/internal/delivery/url_mask_test.go b/internal/delivery/url_mask_test.go new file mode 100644 index 0000000..e6cc152 --- /dev/null +++ b/internal/delivery/url_mask_test.go @@ -0,0 +1,196 @@ +package delivery_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/delivery" +) + +// The path of a Slack incoming webhook URL is the credential: +// whoever holds these segments can post to the channel +// forever. None of them may reach a stored delivery error, +// which lives on disk in the per-webhook database and is +// serialized by the JSON tag on DeliveryResult.Error. +const ( + maskSecretPath = "/services/T00000000/B00000000/" + + "XXXXXXXXXXXXXXXXXXXXXXXX" +) + +// assertNoCredential fails if the whole path or any single +// segment of it survived into the message, so a partial leak +// fails the test too. +func assertNoCredential(t *testing.T, msg string) { + t.Helper() + + segments := []string{ + maskSecretPath, + "services", + "T00000000", + "B00000000", + "XXXXXXXXXXXXXXXXXXXXXXXX", + } + + for _, segment := range segments { + assert.NotContains(t, msg, segment) + } +} + +// storedDeliveryError returns the error string persisted for a +// delivery, which is what an operator and any future API read. +func storedDeliveryError( + t *testing.T, db *gorm.DB, deliveryID string, +) string { + t.Helper() + + var result database.DeliveryResult + + require.NoError(t, db.Where( + "delivery_id = ?", deliveryID, + ).First(&result).Error) + + return result.Error +} + +// deliverSlackTo runs a Slack delivery against webhookURL and +// returns the error string it persisted. +func deliverSlackTo( + t *testing.T, webhookURL string, +) string { + t.Helper() + + db := testWebhookDB(t) + e := testEngine(t, 1) + targetID := uuid.New().String() + + slackCfg, err := json.Marshal( + delivery.SlackTargetConfig{ + WebhookURL: webhookURL, + }, + ) + require.NoError(t, err) + + event := seedEvent(t, db, `{"test":true}`) + + dlv := seedDelivery( + t, db, event.ID, targetID, + database.DeliveryStatusPending, + ) + + d := buildSlackDelivery( + dlv, event, targetID, + "test-slack-mask", string(slackCfg), + ) + + e.ExportDeliverSlack(context.TODO(), db, d) + + assertDeliveryStatus(t, db, dlv.ID, + database.DeliveryStatusFailed, + ) + + return storedDeliveryError(t, db, dlv.ID) +} + +// TestDeliverSlack_TransportErrorMasksWebhookURL is the +// load-bearing regression test: a transport failure must not +// persist the webhook URL's credential into the database, and +// must still say what went wrong and where. +func TestDeliverSlack_TransportErrorMasksWebhookURL( + t *testing.T, +) { + t.Parallel() + + // A server closed before use gives a deterministic + // transport failure against a known host. + ts := httptest.NewServer(http.NewServeMux()) + host := ts.URL + + ts.Close() + + errMsg := deliverSlackTo(t, host+maskSecretPath) + + require.NotEmpty(t, errMsg) + assertNoCredential(t, errMsg) + + // The diagnostic value survives: the operation, the host + // and the transport failure are all still reported, and + // only the path is elided. + assert.Contains(t, errMsg, "sending request") + assert.Contains(t, errMsg, "Post") + assert.Contains(t, errMsg, host+"/...") + assert.Contains(t, errMsg, "connection refused") +} + +// TestDeliverSlack_UnparsableURLMasksWebhookURL covers the +// other error path out of a Slack attempt: url.Parse also +// embeds the whole URL in the error it returns. +func TestDeliverSlack_UnparsableURLMasksWebhookURL( + t *testing.T, +) { + t.Parallel() + + errMsg := deliverSlackTo( + t, + "https://hooks.slack.com"+maskSecretPath+"\n", + ) + + require.NotEmpty(t, errMsg) + assertNoCredential(t, errMsg) + assert.Contains(t, errMsg, "invalid control character") +} + +// TestDoHTTPRequest_TransportErrorMasksURL proves the HTTP +// target's transport errors are masked too; its destination +// URL can carry a token in a query string. +func TestDoHTTPRequest_TransportErrorMasksURL(t *testing.T) { + t.Parallel() + + ts := httptest.NewServer(http.NewServeMux()) + host := ts.URL + + ts.Close() + + e := testEngine(t, 1) + + cfg, err := e.ExportParseHTTPConfig( + newHTTPTargetConfig(host + maskSecretPath), + ) + require.NoError(t, err) + + statusCode, _, _, reqErr := e.ExportDoHTTPRequest( + context.TODO(), cfg, + &database.Event{Body: `{"test":true}`}, + ) + require.Error(t, reqErr) + assert.Zero(t, statusCode) + + assertNoCredential(t, reqErr.Error()) + assert.Contains(t, reqErr.Error(), host+"/...") + assert.Contains( + t, reqErr.Error(), "connection refused", + ) +} + +// TestValidateTargetURL_UnparsableURLIsMasked proves the SSRF +// validator's error does not carry the submitted URL, which +// the handler both logs and shows. +func TestValidateTargetURL_UnparsableURLIsMasked(t *testing.T) { + t.Parallel() + + err := delivery.ValidateTargetURL( + context.TODO(), + "https://hooks.slack.com"+maskSecretPath+"\n", + ) + require.Error(t, err) + + assertNoCredential(t, err.Error()) + assert.Contains(t, err.Error(), "invalid URL") +} diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index b934280..e79d7f5 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -29,10 +29,8 @@ func (h *Handlers) HandleLoginPage() http.HandlerFunc { // HandleLoginSubmit handles the login form submission (POST) func (h *Handlers) HandleLoginSubmit() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - // Limit request body to prevent memory exhaustion - r.Body = http.MaxBytesReader(w, r.Body, 1<= database.RetentionForeverDays { + return database.RetentionForeverDays, nil + } + + if v > database.MaxFiniteRetentionDays { + return 0, errRetentionTooLarge + } + + return v, nil +} + // EventWithDeliveries holds an event and its deliveries. type EventWithDeliveries struct { database.Event - Deliveries []database.Delivery + Deliveries []DeliveryView +} + +// DeliveryView is the display-safe projection of a delivery +// for the event log page. Its target is a TargetView, so the +// stored configuration blob — which holds the target's +// credential — has no path to the template. +type DeliveryView struct { + ID string + Status database.DeliveryStatus + Target delivery.TargetView } // HandleSourceList shows a list of user's webhooks. @@ -106,11 +183,30 @@ func (h *Handlers) buildWebhookListItems( // HandleSourceCreate shows the form to create a new webhook. func (h *Handlers) HandleSourceCreate() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - data := map[string]any{ - tmplKeyError: "", - } + h.renderTemplate( + w, r, "sources_new.html", + newSourceFormData("", "", ""), + ) + } +} - h.renderTemplate(w, r, "sources_new.html", data) +// newSourceFormData builds the template data for the webhook creation +// form. +// +// It carries the retention default so the pre-filled value comes from +// database.DefaultRetentionDays rather than being a third hardcoded +// copy of the same policy, and it carries the submitted name and +// description so that re-rendering the form after a validation failure +// gives the user their input back instead of a blank form. The edit +// form already behaves that way; create now matches it. +func newSourceFormData( + errMsg, name, description string, +) map[string]any { + return map[string]any{ + tmplKeyError: errMsg, + "Name": name, + "Description": description, + "DefaultRetentionDays": database.DefaultRetentionDays, } } @@ -127,10 +223,8 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc { return } - r.Body = http.MaxBytesReader( - w, r.Body, 1< 0 { - retentionDays = v - } + return } h.createWebhookWithEntrypoint( @@ -315,12 +417,17 @@ func (h *Handlers) renderSourceDetail( scheme = fwdProto } + // The template calls Webhook methods, which take pointer + // receivers; html/template cannot address a value stored in a map. data := map[string]any{ - tmplKeyWebhook: webhook, + tmplKeyWebhook: &webhook, "Entrypoints": entrypoints, - "Targets": targets, - "Events": events, - "BaseURL": scheme + "://" + host, + // Targets are projected to a display-safe view: the + // stored config blob holds credentials and must never + // reach a template. + "Targets": delivery.NewTargetViews(targets), + "Events": events, + "BaseURL": scheme + "://" + host, } h.renderTemplate(w, r, "source_detail.html", data) @@ -352,7 +459,7 @@ func (h *Handlers) HandleSourceEdit() http.HandlerFunc { } data := map[string]any{ - tmplKeyWebhook: webhook, + tmplKeyWebhook: &webhook, tmplKeyError: "", } @@ -386,10 +493,8 @@ func (h *Handlers) HandleSourceEditSubmit() http.HandlerFunc { return } - r.Body = http.MaxBytesReader( - w, r.Body, 1< 0 { - webhook.RetentionDays = v - } -} - // HandleSourceDelete handles webhook deletion. func (h *Handlers) HandleSourceDelete() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { @@ -533,6 +637,13 @@ func (h *Handlers) deleteWebhookResources( return } + // Release the delivery engine's per-webhook archiving state + // so a deleted webhook's archive writer (and any handle open + // within its debounce window) does not linger for the + // process lifetime. The archive file itself is deliberately + // left on disk; see evictArchiveWriter. + h.evictArchiveWriter(webhook.ID) + err = h.dbMgr.DeleteDB(webhook.ID) if err != nil { h.log.Error( @@ -551,6 +662,64 @@ func (h *Handlers) deleteWebhookResources( http.Redirect(w, r, "/sources", http.StatusSeeOther) } +// evictArchiveWriter asks the delivery engine to drop its +// cached archive writer for a webhook, closing the archive file +// handle. +// +// The archive database file is NOT deleted. Unlike the event +// database — which is per-webhook working storage and is +// hard-deleted with the webhook — an archive is explicitly +// long-term storage that an operator may want to keep or move +// away for offline retention. Destroying it as a side effect of +// deleting a webhook would be a surprising and unrecoverable +// data loss, so the file is left for the operator to handle. +func (h *Handlers) evictArchiveWriter(webhookID string) { + if h.evictor == nil { + return + } + + h.evictor.EvictWebhook(webhookID) +} + +// evictArchiveWriterIfUnused releases a webhook's archive +// writer once the webhook has no database target left to feed +// it. +// +// It is called after any child resource of a webhook is +// deleted, and is correct without knowing which kind was: it +// evicts only when no database target remains, so deleting one +// of several database targets — or deleting an unrelated +// target type — leaves a still-needed writer alone. When no +// database target ever existed there is no writer and eviction +// is a no-op. Soft-deleted targets are excluded by GORM's +// default scope, so the row just deleted is not counted. +func (h *Handlers) evictArchiveWriterIfUnused(webhookID string) { + var remaining int64 + + err := h.db.DB(). + Model(&database.Target{}). + Where( + "webhook_id = ? AND type = ?", + webhookID, database.TargetTypeDatabase, + ). + Count(&remaining).Error + if err != nil { + h.log.Error( + "failed to count remaining database targets", + "webhook_id", webhookID, + "error", err, + ) + + return + } + + if remaining > 0 { + return + } + + h.evictArchiveWriter(webhookID) +} + // HandleSourceLogs shows the request/response logs for a // webhook. func (h *Handlers) HandleSourceLogs() http.HandlerFunc { @@ -590,7 +759,7 @@ func (h *Handlers) HandleSourceLogs() http.HandlerFunc { } data := map[string]any{ - tmplKeyWebhook: webhook, + tmplKeyWebhook: &webhook, "Events": evts, "Page": page, "TotalPages": totalPages, @@ -605,22 +774,27 @@ func (h *Handlers) HandleSourceLogs() http.HandlerFunc { } } -// loadTargetMap loads targets into a map keyed by target ID. +// loadTargetMap loads targets into a map of display-safe +// views keyed by target ID. The projection happens here so +// that no caller can hand a raw target, configuration blob +// and all, to a template. func (h *Handlers) loadTargetMap( webhookID string, -) map[string]database.Target { +) map[string]delivery.TargetView { var targets []database.Target h.db.DB().Where( "webhook_id = ?", webhookID, ).Find(&targets) + views := delivery.NewTargetViews(targets) + targetMap := make( - map[string]database.Target, len(targets), + map[string]delivery.TargetView, len(views), ) - for _, t := range targets { - targetMap[t.ID] = t + for _, v := range views { + targetMap[v.ID] = v } return targetMap @@ -645,7 +819,7 @@ func (h *Handlers) parsePage(r *http.Request) int { func (h *Handlers) loadEventsWithDeliveries( w http.ResponseWriter, webhook database.Webhook, - targetMap map[string]database.Target, + targetMap map[string]delivery.TargetView, page int, ) ([]EventWithDeliveries, int64) { var totalEvents int64 @@ -684,22 +858,39 @@ func (h *Handlers) loadEventsWithDeliveries( for i := range events { result[i].Event = events[i] + var deliveries []database.Delivery + webhookDB.Where( "event_id = ?", events[i].ID, - ).Find(&result[i].Deliveries) + ).Find(&deliveries) - for j := range result[i].Deliveries { - tid := result[i].Deliveries[j].TargetID - - if target, ok := targetMap[tid]; ok { - result[i].Deliveries[j].Target = target - } - } + result[i].Deliveries = newDeliveryViews( + deliveries, targetMap, + ) } return result, totalEvents } +// newDeliveryViews projects deliveries for rendering, +// resolving each one's target to its display-safe view. +func newDeliveryViews( + deliveries []database.Delivery, + targetMap map[string]delivery.TargetView, +) []DeliveryView { + views := make([]DeliveryView, len(deliveries)) + + for i := range deliveries { + views[i] = DeliveryView{ + ID: deliveries[i].ID, + Status: deliveries[i].Status, + Target: targetMap[deliveries[i].TargetID], + } + } + + return views +} + // HandleEntrypointCreate handles adding a new entrypoint. func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { @@ -725,10 +916,8 @@ func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc { return } - r.Body = http.MaxBytesReader( - w, r.Body, 1<Webhooks`) + assert.Contains( + t, body, `class="btn-text w-full text-left">Webhooks`, + ) + assert.Contains( + t, body, + `

Webhooks

`, + ) + assert.NotContains( + t, body, ">Sources<", + "no user-visible element may still be labelled Sources", + ) + assert.Contains( + t, body, `href="/sources"`, + "the /sources route itself must not change", + ) +} + +// TestEditPageUsesWebhookTerminology pins the edit page's heading and +// its back link. The link's href still points at /source/{id}, which is +// intentional: only user-visible copy changes. +func TestEditPageUsesWebhookTerminology(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + var sess *session.Session + + app := newTestApp(t, &h, &sess) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + // The webhook goes in as a pointer because source_edit.html calls + // Webhook.RetentionLabel, a pointer method: a map element is not + // addressable, so a value here renders an error instead of the + // page. + webhook := &database.Webhook{Name: "wh", RetentionDays: 14} + webhook.ID = testWebhookID + + body := renderPage(t, h, sess, "source_edit.html", map[string]any{ + dataKeyWebhook: webhook, + dataKeyError: "", + }) + + assert.Contains(t, body, "Edit Webhook") + assert.NotContains(t, body, ">Sources<") + assert.Contains(t, body, `href="/source/wh-1"`) +} + +// TestCreateFormRetentionCopyMatchesBehaviour pins the create form's +// retention copy to what the code does: the reaper permanently deletes +// events past the cutoff, an empty field falls back to +// DefaultRetentionDays, and 0 is rewritten to the retain-forever +// sentinel by Webhook.BeforeSave. +func TestCreateFormRetentionCopyMatchesBehaviour(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + var sess *session.Session + + app := newTestApp(t, &h, &sess) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + body := renderPage(t, h, sess, "sources_new.html", map[string]any{ + "Name": "", + "Description": "", + "DefaultRetentionDays": database.DefaultRetentionDays, + dataKeyError: "", + }) + + assert.Contains( + t, body, + "permanently deletes events older than this", + "the form must say retention is enforced by deletion", + ) + assert.Contains(t, body, "Enter 0 to retain events forever") + assert.Contains( + t, body, + "leave blank to use the default of "+ + strconv.Itoa(database.DefaultRetentionDays)+" days", + "blank means the default, not forever", + ) +} + +// TestEditFormRetentionCopyMatchesBehaviour pins the edit form's +// retention copy, including that it states the stored policy via +// RetentionLabel and that an empty field leaves that policy unchanged +// rather than meaning forever. +func TestEditFormRetentionCopyMatchesBehaviour(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + var sess *session.Session + + app := newTestApp(t, &h, &sess) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + finite := &database.Webhook{Name: "wh", RetentionDays: 14} + finite.ID = testWebhookID + + body := renderPage(t, h, sess, "source_edit.html", map[string]any{ + dataKeyWebhook: finite, + dataKeyError: "", + }) + + assert.Contains(t, body, "Currently 14 days.") + assert.Contains( + t, body, + "permanently deletes events older than this", + ) + assert.Contains(t, body, "Enter 0 to retain events forever") + assert.Contains( + t, body, + "leave blank to keep the current setting", + "blank means unchanged, not forever", + ) + + forever := &database.Webhook{ + Name: "wh", + RetentionDays: database.RetentionForeverDays, + } + forever.ID = "wh-2" + + foreverBody := renderPage( + t, h, sess, "source_edit.html", map[string]any{ + dataKeyWebhook: forever, + dataKeyError: "", + }, + ) + + assert.Contains( + t, foreverBody, "Currently forever.", + "a retain-forever webhook must not read as a day count", + ) + assert.Contains( + t, foreverBody, + "No events are deleted while retention is set to forever", + ) + assert.NotContains( + t, foreverBody, + "permanently deletes events older than this", + "the reaper skips retain-forever webhooks, so the form "+ + "must not claim it deletes their events", + ) +} + +// TestEntrypointCopyButtonIsProgressiveEnhancement proves the copy +// affordance degrades: the button ships with the hidden attribute, so a +// browser that never runs app.js shows no dead control, and the URL is +// rendered as ordinary selectable text either way. +func TestEntrypointCopyButtonIsProgressiveEnhancement(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + var sess *session.Session + + app := newTestApp(t, &h, &sess) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + entrypoint := database.Entrypoint{Path: "abc123"} + entrypoint.ID = "ep-1" + + // The webhook goes in as a pointer because source_detail.html + // calls Webhook.RetentionLabel, a pointer method: a map element + // is not addressable, so a value here aborts execution partway + // down the page, after the copy button has already been flushed + // to the response. + webhook := &database.Webhook{Name: "wh", RetentionDays: 14} + webhook.ID = testWebhookID + webhook.CreatedAt = time.Date( + 2026, time.January, 2, 3, 4, 5, 0, time.UTC, + ) + + body := renderPage(t, h, sess, "source_detail.html", map[string]any{ + dataKeyWebhook: webhook, + "Entrypoints": []database.Entrypoint{entrypoint}, + // The handler passes delivery.NewTargetViews(targets), never + // raw targets, so the test data has to have that same shape. + "Targets": delivery.NewTargetViews(nil), + "Events": []database.Event{}, + "BaseURL": "https://hooks.example.com", + }) + + assert.Contains( + t, body, + `