internal/database/retention.go

@@ -184,16 +184,6 @@ func (r *RetentionReaper) sweep(ctx context.Context) {
 
 		wh := webhooks[i]
 
-		// 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
-		}
-
 		// Nothing to reap if the per-webhook database has never
 		// been created.
 		if !r.dbManager.DBExists(wh.ID) {
@@ -212,6 +202,13 @@ func (r *RetentionReaper) reapWebhook(
 	webhookID string,
 	retentionDays int,
 ) {
+	// A retain-forever webhook has no cutoff, so its database is not
+	// even opened.
+	cutoff, ok := retentionCutoff(time.Now(), retentionDays)
+	if !ok {
+		return
+	}
+
 	db, err := r.dbManager.GetDB(webhookID)
 	if err != nil {
 		r.log.Error(
@@ -223,11 +220,6 @@ func (r *RetentionReaper) reapWebhook(
 		return
 	}
 
-	cutoff, ok := retentionCutoff(time.Now(), retentionDays)
-	if !ok {
-		return
-	}
-
 	deleted, err := reapExpired(ctx, db, cutoff)
 	if err != nil {
 		r.log.Error(

internal/database/retention_test.go

@@ -362,7 +362,7 @@ func TestRetentionReaper_HugeFiniteRetentionRetainsRecentEvents(
 		t,
 		overflowingRetentionDays,
 		database.RetentionForeverDays,
-		"the test value must not be rescued by the forever skip",
+		"the test value must not be treated as retain-forever",
 	)
 
 	webhookID := createWebhook(

internal/handlers/source_management.go

@@ -41,71 +41,51 @@ type WebhookListItem struct {
 // errMissingURL signals that a required URL was not provided.
 var errMissingURL = errors.New("missing URL")
 
-// errInvalidRetention signals a retention_days form value that is not
-// a non-negative whole number.
-var errInvalidRetention = errors.New("invalid retention days")
-
-// errRetentionTooLarge signals a retention_days form value that is a
-// whole number but larger than the reaper's cutoff arithmetic can
-// represent. It is distinguished from errInvalidRetention so the form
-// can tell the user the actual ceiling instead of implying their input
-// was not a number.
-var errRetentionTooLarge = errors.New("retention days out of range")
-
-// retentionErrorMessage returns the message the create and edit forms
-// show the user for a rejected retention_days value. Any error other
-// than errRetentionTooLarge falls back to the generic wording, so an
-// unrecognised parse failure still produces a sensible 400 rather than
-// an empty alert.
-func retentionErrorMessage(err error) string {
-	if errors.Is(err, errRetentionTooLarge) {
-		return "Retention must be at most " +
-			strconv.Itoa(database.MaxFiniteRetentionDays) +
-			" days, or 0 to retain events forever."
-	}
-
-	return "Retention must be a whole number of days, or 0 to " +
-		"retain events forever."
-}
-
-// parseRetentionDays interprets a retention_days form value.
+// parseRetentionDays interprets a retention_days form value. It
+// returns the number of days, or, for a value it refuses, the message
+// the create and edit forms show; the message is empty when the value
+// is accepted.
 //
 // An empty value yields fallback, which lets the create path apply the
 // default and the edit path leave the stored value unchanged. A value
 // of 0 is returned as 0 and is rewritten to the retain-forever
 // sentinel by database.Webhook's BeforeSave hook. Anything unparseable
-// or negative is an error rather than a silently substituted default.
+// or negative is refused rather than silently given a default.
 //
 // The upper bound is not cosmetic. The reaper computes its cutoff as a
 // time.Duration, an int64 nanosecond count, so a day count above
 // database.MaxFiniteRetentionDays overflows, puts the cutoff in the
 // future, and deletes every event the webhook has. A finite value
-// above that ceiling is therefore a 400.
+// above that ceiling is therefore refused, and the message names the
+// ceiling rather than implying the input was not a number.
 //
 // A value at or above the retain-forever sentinel is not out of range:
 // it is what the edit form pre-fills for a retain-forever webhook, so
 // submitting the form back unchanged has to keep meaning "forever"
 // rather than being rejected.
-func parseRetentionDays(raw string, fallback int) (int, error) {
+func parseRetentionDays(raw string, fallback int) (int, string) {
 	raw = strings.TrimSpace(raw)
 	if raw == "" {
-		return fallback, nil
+		return fallback, ""
 	}
 
 	v, err := strconv.Atoi(raw)
 	if err != nil || v < 0 {
-		return 0, errInvalidRetention
+		return 0, "Retention must be a whole number of days, or 0 to " +
+			"retain events forever."
 	}
 
 	if v >= database.RetentionForeverDays {
-		return database.RetentionForeverDays, nil
+		return database.RetentionForeverDays, ""
 	}
 
 	if v > database.MaxFiniteRetentionDays {
-		return 0, errRetentionTooLarge
+		return 0, "Retention must be at most " +
+			strconv.Itoa(database.MaxFiniteRetentionDays) +
+			" days, or 0 to retain events forever."
 	}
 
-	return v, nil
+	return v, ""
 }
 
 // DeliveryView is the display-safe projection of a delivery
@@ -361,16 +341,13 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc {
 			return
 		}
 
-		retentionDays, retErr := parseRetentionDays(
+		retentionDays, errMsg := parseRetentionDays(
 			retentionStr, database.DefaultRetentionDays,
 		)
-		if retErr != nil {
+		if errMsg != "" {
 			h.renderTemplateStatus(
 				w, r, "sources_new.html",
-				newSourceFormData(
-					retentionErrorMessage(retErr),
-					name, description,
-				),
+				newSourceFormData(errMsg, name, description),
 				http.StatusBadRequest,
 			)
 
@@ -655,13 +632,13 @@ func (h *Handlers) applyWebhookEdit(
 
 	// An empty field falls back to the stored value, so submitting the
 	// form without touching retention leaves the policy alone.
-	retentionDays, retErr := parseRetentionDays(
+	retentionDays, errMsg := parseRetentionDays(
 		r.PostFormValue("retention_days"), webhook.RetentionDays,
 	)
-	if retErr != nil {
+	if errMsg != "" {
 		data := map[string]any{
 			tmplKeyWebhook: webhook,
-			tmplKeyError:   retentionErrorMessage(retErr),
+			tmplKeyError:   errMsg,
 		}
 
 		h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusBadRequest)

internal/handlers/source_management_test.go

@@ -368,31 +368,42 @@ func TestHandleSourceCreateSubmit_OverflowingRetentionIsRejected(
 // boundary between "too large to represent" and "retain forever": the
 // sentinel is above MaxFiniteRetentionDays, but it is the value the
 // edit form pre-fills, so it must be accepted rather than rejected as
-// out of range.
+// out of range. A value above the sentinel is stored as the sentinel.
 func TestHandleSourceCreateSubmit_SentinelIsAcceptedAsForever(
 	t *testing.T,
 ) {
 	t.Parallel()
 
-	env := setupSourceTest(t)
-	sentinel := strconv.Itoa(database.RetentionForeverDays)
-
-	w := submitCreate(t, env.handlers, env.cookies, "forever", &sentinel)
-	require.Equal(t, http.StatusSeeOther, w.Code)
-
-	wh := onlyWebhook(t, env.db)
-	assert.Equal(
-		t,
+	for _, days := range []int{
 		database.RetentionForeverDays,
-		storedRetentionDays(t, env.db, wh.ID),
-	)
+		database.RetentionForeverDays + 1,
+	} {
+		raw := strconv.Itoa(days)
+
+		t.Run(raw, func(t *testing.T) {
+			t.Parallel()
+
+			env := setupSourceTest(t)
+
+			w := submitCreate(t, env.handlers, env.cookies, "forever", &raw)
+			require.Equal(t, http.StatusSeeOther, w.Code)
+
+			wh := onlyWebhook(t, env.db)
+			assert.Equal(
+				t,
+				database.RetentionForeverDays,
+				storedRetentionDays(t, env.db, wh.ID),
+			)
+		})
+	}
 }
 
 // TestHandleSourceCreateSubmit_RejectedFormKeepsUserInput checks that a
 // validation failure hands the user's typing back, matching what the
 // edit form already does. Losing a long description to a mistyped
 // retention value is the kind of thing that makes people give up on a
-// form.
+// form. Both values carry HTML-special characters, which must come
+// back escaped rather than as markup.
 func TestHandleSourceCreateSubmit_RejectedFormKeepsUserInput(
 	t *testing.T,
 ) {
@@ -401,8 +412,8 @@ func TestHandleSourceCreateSubmit_RejectedFormKeepsUserInput(
 	env := setupSourceTest(t)
 
 	const (
-		name        = "kept-name"
-		description = "a description worth not losing"
+		name        = `kept"><b>name`
+		description = `a </textarea> worth not losing`
 	)
 
 	form := url.Values{}
@@ -419,8 +430,10 @@ func TestHandleSourceCreateSubmit_RejectedFormKeepsUserInput(
 
 	body := w.Body.String()
 
-	assert.Contains(t, body, `value="`+name+`"`)
-	assert.Contains(t, body, description)
+	assert.Contains(t, body, `value="kept&#34;&gt;&lt;b&gt;name"`)
+	assert.Contains(t, body, `a &lt;/textarea&gt; worth not losing`)
+	assert.NotContains(t, body, name)
+	assert.NotContains(t, body, description)
 }
 
 // submitEdit posts the webhook edit form for the given webhook.