From 0ccb01cadacafd7cbb85b97df15cb83235a8ebbe Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Fri, 2 Oct 2026 16:50:34 +0200 Subject: [PATCH] Close the retention follow-ups from the August review (closes #99) Follow-ups from an August review of the retention bounds, each checked against the current tree. A test now pins that a retention value above the keep-forever sentinel is stored as the sentinel. The form's retention parser returns its message directly, so the two error values that were never compared, and the function that mapped them to messages, are gone. The sweep's own keep-forever skip, which duplicated the check in retentionCutoff, is removed; the cutoff is now asked before the webhook's database is opened. The create-form refill test uses HTML-special characters and checks they come back escaped. The README item was already settled; handling for rows made by hand is declined. Model: opus-5-5 --- internal/database/retention.go | 22 +++---- internal/database/retention_test.go | 2 +- internal/handlers/source_management.go | 67 +++++++-------------- internal/handlers/source_management_test.go | 47 +++++++++------ 4 files changed, 60 insertions(+), 78 deletions(-) diff --git a/internal/database/retention.go b/internal/database/retention.go index c34ed99..86544d4 100644 --- a/internal/database/retention.go +++ b/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( diff --git a/internal/database/retention_test.go b/internal/database/retention_test.go index c2dccef..7097677 100644 --- a/internal/database/retention_test.go +++ b/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( diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index b3b4cc8..36a3468 100644 --- a/internal/handlers/source_management.go +++ b/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) diff --git a/internal/handlers/source_management_test.go b/internal/handlers/source_management_test.go index f4437dd..cffd391 100644 --- a/internal/handlers/source_management_test.go +++ b/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">name` + description = `a 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"><b>name"`) + assert.Contains(t, body, `a </textarea> worth not losing`) + assert.NotContains(t, body, name) + assert.NotContains(t, body, description) } // submitEdit posts the webhook edit form for the given webhook.