Stamp the build version into the binary (closes #253)

The binary reported "dev" in every deployment: main.version carried a
placeholder and nothing ever set it. Neither `make build` nor the
Dockerfile passed -X, so a tagged release produced an artifact that
could not say which commit it was, and the upgrade procedure's "confirm
the new build is live" step had nothing to confirm against.

script/version is now the single source of the value: $VERSION when
set, else `git describe --tags --always --dirty`, else "unknown". A
clean checkout at a tag reports exactly that tag; a tree with no git
metadata reports "unknown" rather than failing or naming a tag it may
not be at. Nothing time- or host-dependent is stamped, so two builds of
one commit stay byte-identical.

The Makefile's build target composes the flags -- `-X main.version` plus
whatever GO_LDFLAGS adds -- and every compile goes through it, including
the Dockerfile's static relink, which now contributes its -extldflags
through GO_LDFLAGS instead of replacing -ldflags wholesale. Since
.dockerignore excludes .git/, the image cannot derive the version: it
takes a VERSION build arg, defaulted to "unknown", that script/docker
fills in from the host checkout.

The UI footer needed the other half of the fix. It renders .Version,
which nothing ever put in the template data, so it printed its literal
"dev" fallback no matter what the binary was built as; renderTemplate
now supplies the value on both the map and the wrapper path.

Makefile

@@ -8,14 +8,6 @@
 # unavailable, which is how the Dockerfile passes its build arg in.
 VERSION ?= $(shell script/version)
 
-# An empty override (`make build VERSION=`, or a `--build-arg VERSION=`
-# landing on the Dockerfile's `make build VERSION="$VERSION"`) means unset,
-# exactly as it does in script/version -- stamping "" would leave the binary
-# reporting no version and the footer back on its "dev" fallback. `override`
-# is required: a plain assignment loses to the command-line definition it
-# exists to correct.
-override VERSION := $(or $(strip $(VERSION)),$(shell script/version))
-
 # Extra linker flags for the build target. The static relink in the
 # Dockerfile adds -extldflags here rather than passing its own -ldflags,
 # so composing flags cannot drop the version stamp.

README.md

@@ -719,9 +719,7 @@ reports `unknown` is a build nobody told what it was; it is not a
 failure, but it cannot be traced back to a commit.
 
 `make version` prints what the current checkout would stamp, and
-`make build VERSION=v1.2.3` overrides it. An empty override — from
-`make build VERSION=` or from `--build-arg VERSION=` — means unset
-rather than `""`, and resolves the way an absent one does.
+`make build VERSION=v1.2.3` overrides it.
 
 Nothing that varies between two builds of the same commit is stamped —
 no timestamp, no hostname, no builder identity — so two builds of one
@@ -1723,40 +1721,6 @@ gauge. The outcome counters move only after the status change has been
 written, so a transition the database rejected is never reported as an
 outcome that happened.
 
-#### Inbound HTTP metrics
-
-The middleware records three more on the same registry:
-
-| Metric | Type | Labels |
-| ------ | ---- | ------ |
-| `http_request_duration_seconds` | histogram | `service`, `handler`, `method`, `code` |
-| `http_response_size_bytes` | histogram | `service`, `handler`, `method`, `code` |
-| `http_requests_inflight` | gauge | `service`, `handler` |
-
-Two of those labels are written once per request from bytes the client
-chose, so both are bounded to something this service registers:
-
-- `handler` is the chi route pattern — `/webhook/{uuid}`, never the
-  concrete path. A request matching no route carries `(unmatched)`,
-  and no entrypoint UUID ever reaches a label.
-- `method` is the request method when the router can route it, and
-  `(unmatched)` otherwise. `net/http` accepts any RFC 9110 token as a
-  method, so the raw value bounds the label at nothing; the nine chi
-  matches routes for stay distinguishable, and a token that could only
-  ever have produced a 405 does not get a series of its own.
-
-The other two are not request-controlled: `code` is the status one of
-this service's own handlers wrote, and `service` is a fixed empty
-string.
-
-`http_requests_inflight` is deliberately aggregate — its `handler` is
-always `(all)`, one series counting the requests in flight across the
-whole service. The gauge is incremented before routing and decremented
-after the handler returns, and the route pattern exists only between
-those two moments, so labelling it by pattern would increment one
-series and decrement another, leaving every pattern permanently off by
-the number of requests it served.
-
 ### Rate Limiting
 
 Global blanket rate limiting middleware (e.g., a per-IP throttle shared

internal/delivery/target_config_view.go

@@ -23,13 +23,6 @@ type ConfigField struct {
 	Value string
 }
 
-// deletedNameSuffix marks the name of a target that no longer
-// exists. Deletes are soft and delivery history outlives the
-// target, so the event log shows names of targets that are gone;
-// an operator reading one needs to know it cannot be delivered
-// to, replayed to, or configured.
-const deletedNameSuffix = " (deleted)"
-
 // 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
@@ -37,37 +30,14 @@ const deletedNameSuffix = " (deleted)"
 type TargetView struct {
 	ID     string
 	Name   string
-
-	// Deleted reports that this target's row is soft deleted.
-	// Only views built for historical display carry it set:
-	// every other projection is of a live row.
-	Deleted bool
-
 	Type   database.TargetType
 	Active bool
 	Config []ConfigField
 }
 
-// DisplayName is the name to render, marked when the target has
-// been deleted. Templates showing a name against historical data
-// must use it rather than Name, which stays the stored name.
-func (v TargetView) DisplayName() string {
-	if v.Deleted {
-		return v.Name + deletedNameSuffix
-	}
-
-	return v.Name
-}
-
 // NewTargetViews projects targets for rendering, replacing
 // each stored configuration blob with named, display-safe
 // fields.
-//
-// A soft-deleted row projects exactly as a live one does, minus
-// the deleted marker on its name: masking is a property of the
-// projection, not of the row's state, so a deleted target's
-// credential is as unreachable from a template as a live
-// target's.
 func NewTargetViews(
 	targets []database.Target,
 ) []TargetView {
@@ -79,7 +49,6 @@ func NewTargetViews(
 		views = append(views, TargetView{
 			ID:     t.ID,
 			Name:   t.Name,
-			Deleted: t.DeletedAt.Valid,
 			Type:   t.Type,
 			Active: t.Active,
 			Config: targetConfigFields(t),

internal/delivery/target_config_view_test.go

@@ -2,11 +2,9 @@ package delivery_test
 
 import (
 	"testing"
-	"time"
 
 	"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"
 )
@@ -19,14 +17,6 @@ const (
 	slackWebhookURL = "https://hooks.slack.com" +
 		slackSecretPath
 
-	// slackMaskedURL is what a Slack webhook URL renders as
-	// once masked: scheme and host, path elided.
-	slackMaskedURL = "https://hooks.slack.com/..."
-
-	// slackTargetName is the target name the Slack projection
-	// tests use.
-	slackTargetName = "slack-target"
-
 	viewExampleOrigin = "https://example.com"
 	viewExampleHook   = viewExampleOrigin + "/hook"
 	viewMaskedOrigin  = viewExampleOrigin + "/..."
@@ -43,7 +33,7 @@ func TestMaskedWebhookURL(t *testing.T) {
 	}{
 		"slack webhook": {
 			url:  slackWebhookURL,
-			want: slackMaskedURL,
+			want: "https://hooks.slack.com/...",
 		},
 		"query string dropped": {
 			url:  viewExampleOrigin + "/a?token=secret",
@@ -135,61 +125,23 @@ func viewFor(
 	return views[0]
 }
 
-// TestNewTargetViews_DeletedTarget proves the projection marks
-// a soft-deleted target's name and masks its configuration by
-// the same rules a live target's is. Delivery history outlives
-// the target it names, so this projection is what an operator
-// reads about a target that no longer exists.
-func TestNewTargetViews_DeletedTarget(t *testing.T) {
+func TestNewTargetViews_Slack(t *testing.T) {
 	t.Parallel()
 
-	target := slackTarget()
-	target.DeletedAt = gorm.DeletedAt{
-		Time:  time.Now(),
-		Valid: true,
-	}
-
-	view := viewFor(t, target)
-
-	assert.True(t, view.Deleted)
-	assert.Equal(t, slackTargetName, view.Name)
-	assert.Equal(
-		t, slackTargetName+" (deleted)", view.DisplayName(),
-	)
-	assert.Equal(
-		t,
-		map[string]string{"Webhook URL": slackMaskedURL},
-		fieldMap(view.Config),
-	)
-}
-
-// slackTarget is the live Slack target the projection tests
-// share.
-func slackTarget() database.Target {
-	return database.Target{
-		Name:   slackTargetName,
+	view := viewFor(t, database.Target{
+		Name:   "slack-target",
 		Type:   database.TargetTypeSlack,
 		Active: true,
 		Config: `{"webhookUrl":"` +
 			slackWebhookURL + `"}`,
-	}
-}
-
-func TestNewTargetViews_Slack(t *testing.T) {
-	t.Parallel()
-
-	view := viewFor(t, slackTarget())
-
-	assert.Equal(t, slackTargetName, view.Name)
-
-	// A live target is never marked, so the marker cannot
-	// reach a name that still exists.
-	assert.False(t, view.Deleted)
-	assert.Equal(t, slackTargetName, view.DisplayName())
+	})
 
+	assert.Equal(t, "slack-target", view.Name)
 	assert.Equal(
 		t,
-		map[string]string{"Webhook URL": slackMaskedURL},
+		map[string]string{
+			"Webhook URL": "https://hooks.slack.com/...",
+		},
 		fieldMap(view.Config),
 	)
 }
@@ -260,7 +212,7 @@ func TestNewTargetViews_HTTPMasksDestinationURL(t *testing.T) {
 
 	assert.Equal(
 		t,
-		slackMaskedURL,
+		"https://hooks.slack.com/...",
 		fields["Destination URL"],
 	)
 

internal/handlers/delivery_replay.go

@@ -368,7 +368,7 @@ func (h *Handlers) finishReplay(
 	// The page is read from the form rather than the query string:
 	// this is a POST, and its query string is what logs and Referer
 	// headers record.
-	if page := pageOrFirst(
+	if page := parseNonNegativeInt(
 		r.PostFormValue("page"),
 	); page > 1 {
 		dest += "&page=" + strconv.Itoa(page)

internal/handlers/event_resubmit.go

@@ -267,7 +267,7 @@ func (h *Handlers) finishResubmit(
 	// The page is read from the form rather than the query string:
 	// this is a POST, and its query string is what logs and Referer
 	// headers record.
-	if page := pageOrFirst(
+	if page := parseNonNegativeInt(
 		r.PostFormValue("page"),
 	); page > 1 {
 		dest += "&page=" + strconv.Itoa(page)

internal/handlers/export_test.go

@@ -27,17 +27,6 @@ const MaxRenderedResponseBytesForTest = maxRenderedResponseBytes
 // per-delivery attempt ceiling to the handlers_test package.
 const MaxRenderedAttemptsForTest = maxRenderedAttempts
 
-// MaxTargetRetriesForTest exposes the target max_retries ceiling to
-// the handlers_test package, so the tests assert against the constant
-// the handlers enforce rather than a number copied beside it.
-const MaxTargetRetriesForTest = maxTargetRetries
-
-// PageOrFirstForTest exposes pageOrFirst for use in the handlers_test
-// package.
-func PageOrFirstForTest(s string) int {
-	return pageOrFirst(s)
-}
-
 // DummyVerificationsForTest reports how many equivalent-cost
 // verifications were charged for usernames that do not exist. It
 // lets a test prove the anti-enumeration path ran without timing

internal/handlers/source_logs_deleted_target_test.go

@@ -1,144 +0,0 @@
-package handlers_test
-
-import (
-	"net/http"
-	"net/http/httptest"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/handlers"
-	"sneak.berlin/go/webhooker/internal/session"
-)
-
-// deletedMarker is the suffix the event log appends to the name
-// of a target that no longer exists.
-const deletedMarker = " (deleted)"
-
-// deleteTargetThroughHandler removes a target through the real
-// deletion handler, so the test soft-deletes exactly the way the
-// UI does rather than by writing the timestamp itself.
-func deleteTargetThroughHandler(
-	t *testing.T,
-	h *handlers.Handlers,
-	sess *session.Session,
-	webhookID, targetID string,
-) {
-	t.Helper()
-
-	req := postRequest(
-		"/source/"+webhookID+"/targets/"+targetID+"/delete",
-		authenticatedCookies(
-			t, sess, deleteTestUserID, deleteTestUsername,
-		),
-		map[string]string{
-			paramSourceID: webhookID,
-			paramTargetID: targetID,
-		},
-	)
-	w := httptest.NewRecorder()
-
-	h.HandleTargetDelete().ServeHTTP(w, req)
-
-	require.Equal(t, http.StatusSeeOther, w.Code)
-}
-
-// TestHandleSourceLogs_NamesDeletedTarget proves a delivery
-// produced by a since-deleted target still names it on the event
-// log, marked as deleted.
-//
-// Deletes are soft and deliveries carry no foreign key to the
-// target row, so the history outlives the target. Against a
-// scoped lookup the delivery resolves to a zero view and the page
-// renders ": delivered" with nothing saying what it was delivered
-// to.
-func TestHandleSourceLogs_NamesDeletedTarget(t *testing.T) {
-	t.Parallel()
-
-	var (
-		h     *handlers.Handlers
-		sess  *session.Session
-		db    *database.Database
-		dbMgr *database.WebhookDBManager
-	)
-
-	app := newTestApp(t, &h, &sess, &db, &dbMgr)
-	app.RequireStart()
-
-	t.Cleanup(app.RequireStop)
-
-	wh := seedWebhook(t, db)
-	tgt := seedTarget(t, db, wh.ID, database.TargetTypeLog)
-
-	seedDeliveredEvent(t, dbMgr, wh.ID, tgt.ID)
-
-	// The control: the name is on the page while the target
-	// lives, and is not yet marked as deleted.
-	before := renderSourceLogsPage(t, h, sess, wh.ID)
-	assert.Contains(t, before, tgt.Name)
-	assert.NotContains(t, before, tgt.Name+deletedMarker)
-
-	deleteTargetThroughHandler(t, h, sess, wh.ID, tgt.ID)
-
-	after := renderSourceLogsPage(t, h, sess, wh.ID)
-
-	assert.Contains(
-		t, after, tgt.Name+deletedMarker,
-		"a delivery from a deleted target must keep its name, "+
-			"marked as no longer existing",
-	)
-	assert.Contains(
-		t, after, "delivered",
-		"the delivery history itself must survive the delete",
-	)
-}
-
-// TestHandleSourceLogs_MasksDeletedTargetConfig proves that
-// naming a deleted target does not widen what the page shows of
-// it: its stored configuration stays masked by exactly the rules
-// a live target's is.
-//
-// The lookup behind the name reads soft-deleted rows, so it
-// carries a full target row — credential blob included — into the
-// place a zero value used to sit. The projection to TargetView is
-// what keeps that blob away from the template, and it must hold
-// for a deleted row too.
-func TestHandleSourceLogs_MasksDeletedTargetConfig(t *testing.T) {
-	t.Parallel()
-
-	var (
-		h     *handlers.Handlers
-		sess  *session.Session
-		db    *database.Database
-		dbMgr *database.WebhookDBManager
-	)
-
-	app := newTestApp(t, &h, &sess, &db, &dbMgr)
-	app.RequireStart()
-
-	t.Cleanup(app.RequireStop)
-
-	wh := seedWebhook(t, db)
-	tgt := seedConfiguredTarget(
-		t, db, wh.ID,
-		database.TargetTypeSlack,
-		`{"webhookUrl":"`+slackWebhookURL+`"}`,
-	)
-
-	seedDeliveredEvent(t, dbMgr, wh.ID, tgt.ID)
-	deleteTargetThroughHandler(t, h, sess, wh.ID, tgt.ID)
-
-	body := renderSourceLogsPage(t, h, sess, wh.ID)
-
-	assert.NotContains(t, body, slackSecretPath)
-	assert.NotContains(t, body, "T00000000")
-	assert.NotContains(t, body, "B00000000")
-	assert.NotContains(
-		t, body, "XXXXXXXXXXXXXXXXXXXXXXXX",
-	)
-	assert.NotContains(t, body, "webhookUrl")
-
-	// The name is there; only the credential is not.
-	assert.Contains(t, body, tgt.Name+deletedMarker)
-}

internal/handlers/source_management.go

@@ -860,16 +860,11 @@ func (h *Handlers) HandleSourceLogs() http.HandlerFunc {
 //
 // The load is Unscoped because deleting a target only soft
 // deletes the row while its deliveries survive in the
-// per-webhook database. Both halves of the map need those rows:
-// a scoped load leaves an old delivery with a zero redactor,
-// which renders its response bodies unredacted, and with a zero
-// view, which renders its target as a blank name.
-//
-// This map is historical display only. It is built for the event
-// log page and reaches nothing but DeliveryView.Target: the
-// target list on the source detail page, the edit form and the
-// replay path each resolve targets themselves, and a deleted row
-// is refused there as before.
+// per-webhook database: a scoped load leaves those deliveries
+// with a zero redactor, which renders their response bodies
+// unredacted. Only the redactor half of the map is built from
+// deleted rows. The view half, which is what the page lists,
+// stays scoped.
 func (h *Handlers) loadTargetMap(
 	webhookID string,
 ) (map[string]eventLogTarget, error) {
@@ -885,18 +880,21 @@ func (h *Handlers) loadTargetMap(
 	targetMap := make(
 		map[string]eventLogTarget, len(targets),
 	)
+	live := make([]database.Target, 0, len(targets))
 
 	for i := range targets {
 		targetMap[targets[i].ID] = eventLogTarget{
 			Redactor: delivery.NewRedactor(&targets[i]),
 		}
+
+		if !targets[i].DeletedAt.Valid {
+			live = append(live, targets[i])
+		}
 	}
 
 	// The views come from NewTargetViews rather than being
-	// rebuilt here, so the masking rules stay in one place and a
-	// deleted target's configuration is masked by the same code
-	// that masks a live one's.
-	for _, v := range delivery.NewTargetViews(targets) {
+	// rebuilt here, so the masking rules stay in one place.
+	for _, v := range delivery.NewTargetViews(live) {
 		entry := targetMap[v.ID]
 		entry.View = v
 		targetMap[v.ID] = entry
@@ -907,7 +905,16 @@ func (h *Handlers) loadTargetMap(
 
 // parsePage extracts a page number from the query string.
 func (h *Handlers) parsePage(r *http.Request) int {
-	return pageOrFirst(r.URL.Query().Get("page"))
+	page := 1
+
+	if p := r.URL.Query().Get("page"); p != "" {
+		v, err := strconv.Atoi(p)
+		if err == nil && v > 0 {
+			page = v
+		}
+	}
+
+	return page
 }
 
 // loadEventsWithDeliveries loads paginated events and their
@@ -1436,6 +1443,7 @@ func (h *Handlers) processTargetCreate(
 	// Referer headers and error trackers record.
 	name := r.PostFormValue("name")
 	targetType := database.TargetType(r.PostFormValue("type"))
+	maxRetriesStr := r.PostFormValue("max_retries")
 
 	if name == "" {
 		http.Error(
@@ -1461,14 +1469,7 @@ func (h *Handlers) processTargetCreate(
 		return
 	}
 
-	// A new target has no stored retry count, so an absent field
-	// takes the fire-and-forget default. A field the operator filled
-	// in with something invalid is rejected rather than becoming
-	// that default.
-	maxRetries, ok := targetMaxRetries(w, r, 0)
-	if !ok {
-		return
-	}
+	maxRetries := parseNonNegativeInt(maxRetriesStr)
 
 	target := &database.Target{
 		WebhookID:  webhook.ID,
@@ -1504,22 +1505,19 @@ func isValidTargetType(tt database.TargetType) bool {
 	}
 }
 
-// pageOrFirst parses a paginated page number, answering 1 for
-// anything empty, unparseable or out of range.
-//
-// Falling back rather than rejecting is correct here and only here:
-// a page number is where to send the browser next, not configuration
-// the operator is storing, and the actions that submit one have
-// already completed by the time it is read — answering 400 would
-// report a failure that did not happen. Anything an operator SETS
-// must be validated instead; see parseMaxRetries.
-func pageOrFirst(s string) int {
-	v, err := strconv.Atoi(strings.TrimSpace(s))
-	if err != nil || v < 1 {
-		return 1
+// parseNonNegativeInt parses s as a non-negative integer,
+// returning 0 if s is empty or invalid.
+func parseNonNegativeInt(s string) int {
+	if s == "" {
+		return 0
 	}
 
+	v, err := strconv.Atoi(s)
+	if err == nil && v >= 0 {
 		return v
+	}
+
+	return 0
 }
 
 // targetFormInput carries the raw form values describing a target's

internal/handlers/target_edit.go

@@ -133,28 +133,20 @@ func (h *Handlers) applyTargetEdit(
 		return
 	}
 
+	target.Name = name
+	target.Config = configJSON
+
 	// Retries are offered only by the forms for target types that
 	// retry, so an absent field means "this form does not edit
 	// retries" rather than "set them to zero". Reading it
 	// unconditionally would silently disable retries on any target
 	// saved from a form that does not render the input.
-	//
-	// A field that IS submitted but does not parse is a 400, through
-	// the same validator the create path uses. It is rejected before
-	// anything is written, so a typo cannot destroy the retry count
-	// the target is already delivering with.
 	if r.PostForm.Has("max_retries") {
-		retries, ok := targetMaxRetries(w, r, target.MaxRetries)
-		if !ok {
-			return
+		target.MaxRetries = parseNonNegativeInt(
+			r.PostFormValue("max_retries"),
+		)
 	}
 
-		target.MaxRetries = retries
-	}
-
-	target.Name = name
-	target.Config = configJSON
-
 	err = h.db.DB().Save(target).Error
 	if err != nil {
 		h.serverError(w, "failed to update target", err)

internal/handlers/target_retries.go

@@ -1,119 +0,0 @@
-package handlers
-
-import (
-	"errors"
-	"net/http"
-	"strconv"
-	"strings"
-)
-
-// maxTargetRetries bounds a target's max_retries.
-//
-// Both target forms already declare max="20" on the input, so this
-// enforces server-side what the UI has always advertised rather than
-// introducing a new limit.
-//
-// The number is not cosmetic. Every attempt writes a delivery_results
-// row that the event log then loads and renders, and the engine backs
-// off by 2^(n-1) seconds, so attempt 20 is already about six days
-// after the first. A value beyond this buys no additional durability
-// and only costs rows.
-const maxTargetRetries = 20
-
-// Errors returned when a max_retries form value cannot be turned into
-// a retry count.
-var (
-	// errRetriesInvalid signals a max_retries form value that is not
-	// a non-negative whole number.
-	errRetriesInvalid = errors.New(
-		"retries must be a whole number of attempts",
-	)
-
-	// errRetriesTooLarge signals a max_retries form value that is a
-	// whole number but above maxTargetRetries. It is distinguished
-	// from errRetriesInvalid so the message can name the ceiling
-	// instead of implying the input was not a number.
-	errRetriesTooLarge = errors.New("retries out of range")
-)
-
-// parseMaxRetries interprets a max_retries form value.
-//
-// An ABSENT value — the field empty or not submitted — yields
-// fallback, which lets the create path apply its default and the edit
-// path leave the stored value alone. A value that is SET BUT INVALID
-// is an error: unparseable, negative, or above maxTargetRetries.
-//
-// The distinction is the whole point of this function. max_retries=0
-// means fire-and-forget, so returning 0 for input the operator typed
-// but that did not parse silently disables retries on a
-// store-and-forward proxy — and on the edit path it destroys a
-// working retry configuration over a typo. A default answers a
-// question that was not asked; it never answers one that was asked
-// badly.
-//
-// A target stored with a count above the ceiling before this
-// validation existed keeps rendering and keeps delivering — nothing
-// clamps the row. Re-saving it from the edit form does have to bring
-// it into range, because the form submits the pre-filled value back
-// and accepting it would be the ceiling not applying to the edit
-// path. The 400 names the ceiling, so the fix is one field.
-func parseMaxRetries(raw string, fallback int) (int, error) {
-	raw = strings.TrimSpace(raw)
-	if raw == "" {
-		return fallback, nil
-	}
-
-	v, err := strconv.Atoi(raw)
-	if err != nil || v < 0 {
-		return 0, errRetriesInvalid
-	}
-
-	if v > maxTargetRetries {
-		return 0, errRetriesTooLarge
-	}
-
-	return v, nil
-}
-
-// retriesErrorMessage returns the message the create and edit forms
-// show for a rejected max_retries value. Any error other than
-// errRetriesTooLarge falls back to the generic wording, so an
-// unrecognised parse failure still produces a sensible 400.
-func retriesErrorMessage(err error) string {
-	if errors.Is(err, errRetriesTooLarge) {
-		return errRetriesTooLarge.Error() +
-			": at most " + strconv.Itoa(maxTargetRetries) +
-			" retries"
-	}
-
-	return errRetriesInvalid.Error() +
-		", or 0 for fire-and-forget"
-}
-
-// targetMaxRetries reads and validates max_retries from a target form
-// submission, answering the request with a 400 and reporting false
-// when the value is set but invalid.
-//
-// Both the create and the edit path go through here, so the two
-// cannot come to disagree about what a valid retry count is. The
-// wording matches the timeout control on the same submission.
-func targetMaxRetries(
-	w http.ResponseWriter,
-	r *http.Request,
-	fallback int,
-) (int, bool) {
-	retries, err := parseMaxRetries(
-		r.PostFormValue("max_retries"), fallback,
-	)
-	if err != nil {
-		http.Error(
-			w,
-			"Invalid max retries: "+retriesErrorMessage(err),
-			http.StatusBadRequest,
-		)
-
-		return 0, false
-	}
-
-	return retries, true
-}

internal/handlers/target_retries_test.go

@@ -1,402 +0,0 @@
-package handlers_test
-
-import (
-	"net/http"
-	"net/url"
-	"strconv"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/handlers"
-)
-
-// retriesTargetURL is the destination the retry-validation targets
-// point at. It is a literal public address rather than a hostname so
-// the SSRF check resolves nothing and a sandbox without DNS cannot
-// make these cases pass or fail for the wrong reason.
-const retriesTargetURL = "https://93.184.216.34/hooks/retries"
-
-const (
-	// wayAboveCeiling is the typo'd-extra-zero case from the report.
-	wayAboveCeiling = "999999999"
-
-	// notANumber is the plainest garbage an operator can type, and
-	// the value the report submitted on the edit form.
-	notANumber = "abc"
-
-	// workingRetries is the retry count a seeded target is already
-	// delivering with, which a rejected submission must not disturb.
-	workingRetries = 2
-)
-
-// aboveCeiling is the smallest rejected whole number.
-func aboveCeiling() string {
-	return strconv.Itoa(handlers.MaxTargetRetriesForTest + 1)
-}
-
-// overCeilingRetries is whole-number input past the limit, which is
-// rejected with the limit named.
-func overCeilingRetries() []string {
-	return []string{aboveCeiling(), wayAboveCeiling}
-}
-
-// unparseableRetries is input an operator can type into the field
-// that is not a retry count. Each must be REJECTED: silently reading
-// any of them as 0 turns a store-and-forward proxy into
-// fire-and-forget without saying so.
-//
-// The twenty-digit case is here because it parses as digits but
-// overflows int, which is the one failure the field's own min/max
-// attributes cannot describe.
-func unparseableRetries() []string {
-	return []string{
-		notANumber,
-		"2.7",
-		"-5",
-		"12345678901234567890",
-		"1e3",
-	}
-}
-
-// createRetriesForm is a complete, otherwise-valid HTTP target
-// creation, so the only thing any case below varies is max_retries.
-func createRetriesForm(retries string) url.Values {
-	form := url.Values{}
-	form.Set("name", "retries-target")
-	form.Set("type", string(database.TargetTypeHTTP))
-	form.Set("url", retriesTargetURL)
-
-	if retries != absentField {
-		form.Set("max_retries", retries)
-	}
-
-	return form
-}
-
-// absentField marks a field the form does not submit at all, which is
-// the case that legitimately takes a default and must stay distinct
-// from a field submitted with garbage in it.
-const absentField = "\x00absent"
-
-// absentRetries is every way of saying "the operator did not set
-// this", each of which takes the default rather than a 400. Blank and
-// whitespace-only count as absent here because they do in the timeout
-// and retention controls on the same forms; a rule the fields do not
-// share would be its own surprise.
-func absentRetries() []string {
-	return []string{absentField, "", "  "}
-}
-
-// createWithRetries posts the target create form for a fresh webhook
-// and returns the webhook and the response.
-func createWithRetries(
-	t *testing.T,
-	env *sourceTestEnv,
-	retries string,
-) (database.Webhook, int, string) {
-	t.Helper()
-
-	webhook := seedWebhookWithRetention(t, env.db, 30)
-
-	w := serveTarget(
-		env, http.MethodPost,
-		"/source/"+webhook.ID+"/targets",
-		createRetriesForm(retries),
-	)
-
-	return webhook, w.Code, w.Body.String()
-}
-
-// TestTargetCreate_RetriesAboveCeilingRejected proves the create form
-// enforces a ceiling at all, and that the 400 names it — a rejection
-// that does not say what the limit is leaves the operator guessing.
-func TestTargetCreate_RetriesAboveCeilingRejected(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-	ceiling := strconv.Itoa(handlers.MaxTargetRetriesForTest)
-
-	for _, retries := range overCeilingRetries() {
-		webhook, code, body := createWithRetries(t, env, retries)
-
-		assert.Equal(t, http.StatusBadRequest, code,
-			"max_retries=%s should be rejected", retries)
-		assert.Contains(t, body, ceiling,
-			"the rejection for %s should name the ceiling",
-			retries)
-		assert.Empty(t,
-			targetsForWebhook(t, env.db, webhook.ID),
-			"no target should be created for %s", retries)
-	}
-}
-
-// TestTargetCreate_UnparseableRetriesRejected is the core of the
-// defect: each of these was accepted with HTTP 200 and stored as 0.
-func TestTargetCreate_UnparseableRetriesRejected(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-
-	for _, retries := range unparseableRetries() {
-		webhook, code, body := createWithRetries(t, env, retries)
-
-		assert.Equal(t, http.StatusBadRequest, code,
-			"max_retries=%q should be rejected, not coerced",
-			retries)
-		assert.Contains(t, body, "whole number",
-			"the rejection for %q should say why", retries)
-		assert.Empty(t,
-			targetsForWebhook(t, env.db, webhook.ID),
-			"no target should be created for %q", retries)
-	}
-}
-
-// TestTargetCreate_ValidRetriesStored covers the accepting half,
-// including the ceiling itself: a bound that rejects its own limit
-// would make the advertised maximum unreachable.
-func TestTargetCreate_ValidRetriesStored(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-
-	for _, want := range []int{0, 3, handlers.MaxTargetRetriesForTest} {
-		webhook, code, body := createWithRetries(
-			t, env, strconv.Itoa(want),
-		)
-
-		require.Equal(t, http.StatusSeeOther, code, body)
-
-		targets := targetsForWebhook(t, env.db, webhook.ID)
-		require.Len(t, targets, 1)
-		assert.Equal(t, want, targets[0].MaxRetries)
-	}
-}
-
-// TestTargetCreate_AbsentRetriesTakesDefault keeps the two cases
-// distinct. An omitted field is not an operator asking for something
-// invalid, so it still gets the fire-and-forget default rather than a
-// 400 — otherwise the fix above would make the form unusable.
-func TestTargetCreate_AbsentRetriesTakesDefault(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-
-	for _, retries := range absentRetries() {
-		webhook, code, body := createWithRetries(t, env, retries)
-
-		require.Equal(t, http.StatusSeeOther, code, body)
-
-		targets := targetsForWebhook(t, env.db, webhook.ID)
-		require.Len(t, targets, 1)
-		assert.Equal(t, 0, targets[0].MaxRetries,
-			"an absent max_retries should take the default")
-	}
-}
-
-// seedRetriesTarget creates an HTTP target already delivering with
-// workingRetries retries, through the real create handler.
-func seedRetriesTarget(
-	t *testing.T,
-	env *sourceTestEnv,
-) (database.Webhook, database.Target) {
-	t.Helper()
-
-	webhook, code, body := createWithRetries(
-		t, env, strconv.Itoa(workingRetries),
-	)
-	require.Equal(t, http.StatusSeeOther, code, body)
-
-	targets := targetsForWebhook(t, env.db, webhook.ID)
-	require.Len(t, targets, 1)
-	require.Equal(t, workingRetries, targets[0].MaxRetries)
-
-	return webhook, targets[0]
-}
-
-// editRetriesForm is a complete edit submission that changes the
-// target's name as well, so a rejected submission can be shown to
-// have written nothing at all rather than merely to have left
-// max_retries alone.
-func editRetriesForm(retries string) url.Values {
-	form := url.Values{}
-	form.Set("name", "renamed-by-edit")
-	form.Set("url", retriesTargetURL)
-
-	if retries != absentField {
-		form.Set("max_retries", retries)
-	}
-
-	return form
-}
-
-// assertEditRejectedAndUnchanged submits an edit expected to fail and
-// checks both halves of the requirement: the 400 explains itself, and
-// the target it was submitted against is untouched.
-func assertEditRejectedAndUnchanged(
-	t *testing.T,
-	env *sourceTestEnv,
-	retries, wantReason string,
-) {
-	t.Helper()
-
-	webhook, target := seedRetriesTarget(t, env)
-
-	w := submitTargetEdit(
-		env, webhook.ID, target.ID, editRetriesForm(retries),
-	)
-
-	assert.Equal(t, http.StatusBadRequest, w.Code,
-		"max_retries=%q should be rejected on edit", retries)
-	assert.Contains(t, w.Body.String(), wantReason,
-		"the rejection for %q should say why", retries)
-
-	stored := storedTarget(t, env, target.ID)
-	assert.Equal(t, workingRetries, stored.MaxRetries,
-		"a rejected edit must not destroy the working retry "+
-			"count with %q", retries)
-	assert.Equal(t, "retries-target", stored.Name,
-		"a rejected edit must write nothing at all")
-}
-
-// TestTargetEdit_UnparseableRetriesRejected is the damaging half of
-// the defect. A target delivering with two retries, re-saved with a
-// typo in the field, returned 200 and was left with retries disabled
-// and nothing said.
-func TestTargetEdit_UnparseableRetriesRejected(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-
-	for _, retries := range unparseableRetries() {
-		assertEditRejectedAndUnchanged(
-			t, env, retries, "whole number",
-		)
-	}
-}
-
-// TestTargetEdit_RetriesAboveCeilingRejected proves the ceiling
-// applies to the edit path too, naming itself, so the two paths
-// cannot disagree about what is storable.
-func TestTargetEdit_RetriesAboveCeilingRejected(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-	ceiling := strconv.Itoa(handlers.MaxTargetRetriesForTest)
-
-	for _, retries := range overCeilingRetries() {
-		assertEditRejectedAndUnchanged(t, env, retries, ceiling)
-	}
-}
-
-// TestTargetEdit_ValidRetriesStored covers the accepting half of the
-// edit path, so the ceiling cannot be enforced by simply refusing
-// every submission.
-func TestTargetEdit_ValidRetriesStored(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-
-	for _, want := range []int{0, 9, handlers.MaxTargetRetriesForTest} {
-		webhook, target := seedRetriesTarget(t, env)
-
-		w := submitTargetEdit(
-			env, webhook.ID, target.ID,
-			editRetriesForm(strconv.Itoa(want)),
-		)
-		require.Equal(t,
-			http.StatusSeeOther, w.Code, w.Body.String(),
-		)
-
-		assert.Equal(t, want,
-			storedTarget(t, env, target.ID).MaxRetries)
-	}
-}
-
-// TestTargetEdit_AbsentRetriesLeavesStoredValue is the edit path's
-// absent-versus-invalid case. Retries are only offered by the forms
-// for types that retry, so a submission without the field must leave
-// the stored count alone rather than be rejected or zeroed.
-func TestTargetEdit_AbsentRetriesLeavesStoredValue(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-
-	for _, retries := range absentRetries() {
-		webhook, target := seedRetriesTarget(t, env)
-
-		w := submitTargetEdit(
-			env, webhook.ID, target.ID,
-			editRetriesForm(retries),
-		)
-		require.Equal(t,
-			http.StatusSeeOther, w.Code, w.Body.String(),
-		)
-
-		assert.Equal(t, workingRetries,
-			storedTarget(t, env, target.ID).MaxRetries,
-			"an absent max_retries must leave the stored "+
-				"count alone (%q)", retries)
-	}
-}
-
-// TestTargetRetries_CreateAndEditAgreeOnEveryCase proves the two
-// paths cannot disagree, which is what let the create form and the
-// edit form drift apart in the first place. Every input is submitted
-// to both and the accept/reject verdicts are compared.
-func TestTargetRetries_CreateAndEditAgreeOnEveryCase(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-
-	accepted := []string{
-		"0", "1",
-		strconv.Itoa(handlers.MaxTargetRetriesForTest),
-	}
-	overCeiling := overCeilingRetries()
-	unparseable := unparseableRetries()
-
-	cases := make(
-		[]string, 0,
-		len(accepted)+len(overCeiling)+len(unparseable),
-	)
-	cases = append(cases, accepted...)
-	cases = append(cases, overCeiling...)
-	cases = append(cases, unparseable...)
-
-	for _, retries := range cases {
-		_, createCode, _ := createWithRetries(t, env, retries)
-
-		webhook, target := seedRetriesTarget(t, env)
-		editCode := submitTargetEdit(
-			env, webhook.ID, target.ID,
-			editRetriesForm(retries),
-		).Code
-
-		assert.Equal(t,
-			createCode == http.StatusBadRequest,
-			editCode == http.StatusBadRequest,
-			"create and edit must agree on max_retries=%q "+
-				"(create %d, edit %d)",
-			retries, createCode, editCode,
-		)
-	}
-}
-
-// TestPageOrFirst_CoercesRatherThanRejects pins the one place a
-// non-numeric form value legitimately falls back. A page number says
-// where to send the browser after an action that has already
-// happened, so it is not configuration and rejecting it would report
-// a failure that did not occur.
-func TestPageOrFirst_CoercesRatherThanRejects(t *testing.T) {
-	t.Parallel()
-
-	for _, s := range []string{"", "abc", "0", "-1", "2.7", " "} {
-		assert.Equal(t, 1, handlers.PageOrFirstForTest(s),
-			"%q should fall back to the first page", s)
-	}
-
-	assert.Equal(t, 4, handlers.PageOrFirstForTest("4"))
-	assert.Equal(t, 4, handlers.PageOrFirstForTest(" 4 "))
-}

internal/middleware/export_test.go

@@ -4,32 +4,8 @@ import (
 	"context"
 	"net/http"
 	"time"
-
-	httpmetrics "github.com/slok/go-http-metrics/metrics"
 )
 
-// MetricsMiddlewareForTest builds the metrics recording middleware
-// against a caller-supplied recorder, so a test can gather from its
-// own Prometheus registry rather than the process-wide default one
-// that Middleware.Metrics uses.
-func MetricsMiddlewareForTest(
-	rec httpmetrics.Recorder,
-) func(http.Handler) http.Handler {
-	return metricsMiddleware(rec)
-}
-
-// UnmatchedRouteConst exposes the sentinel that stands in for a
-// request matching no route pattern.
-const UnmatchedRouteConst = unmatchedRoute
-
-// InflightHandlerConst exposes the fixed handler label on the
-// inflight gauge.
-const InflightHandlerConst = inflightHandler
-
-// UnmatchedMethodConst exposes the sentinel that stands in for a
-// method the router can never route.
-const UnmatchedMethodConst = unmatchedMethod
-
 // NewLoggingResponseWriterForTest wraps newLoggingResponseWriter
 // for use in external test packages.
 func NewLoggingResponseWriterForTest(

internal/middleware/metrics.go

@@ -1,182 +0,0 @@
-package middleware
-
-import (
-	"context"
-	"net/http"
-	"time"
-
-	"github.com/go-chi/chi"
-	httpmetrics "github.com/slok/go-http-metrics/metrics"
-	prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
-	ghmm "github.com/slok/go-http-metrics/middleware"
-	"github.com/slok/go-http-metrics/middleware/std"
-)
-
-// inflightHandler is the fixed `handler` label on
-// http_requests_inflight, the one HTTP metric here that cannot carry
-// a route pattern.
-//
-// The gauge is incremented before the wrapped handler runs and
-// decremented after it returns, and the pattern only exists between
-// those two moments. Deriving the label from the route would
-// therefore increment one series and decrement another, leaving every
-// pattern permanently off by the number of requests it served — a
-// broken gauge, on top of the per-path cardinality this file exists
-// to remove. So the gauge is deliberately aggregate: one series,
-// counting the requests in flight across the whole service.
-const inflightHandler = "(all)"
-
-// unmatchedMethod is the `method` label for a request whose method
-// the router can never route.
-//
-// It is deliberately the same sentinel as unmatchedRoute rather than
-// a spelling of its own: both stand for a client-chosen token that
-// matched nothing this service registers, and giving one idea two
-// spellings would read in a scrape as two different unmatched states.
-const unmatchedMethod = unmatchedRoute
-
-// routePatternID is the `handler` label for a request: the chi route
-// pattern, never the concrete path.
-//
-// The pattern is what bounds the label's domain to the routes the
-// service registers. The path does not bound it at all — every byte
-// after /webhook/ is client-chosen, so labelling by path lets any
-// unauthenticated client mint permanent series at will, and publishes
-// the entrypoint UUID (the receiver's only credential) in the scrape
-// while doing it.
-//
-// chi populates the route context during routeHTTP, so this is only
-// valid once routing has run. Every caller below is on the recording
-// side of the middleware, which go-http-metrics defers until after
-// the wrapped handler returns.
-func routePatternID(ctx context.Context) string {
-	if rc := chi.RouteContext(ctx); rc != nil {
-		if pattern := rc.RoutePattern(); pattern != "" {
-			return pattern
-		}
-	}
-
-	return unmatchedRoute
-}
-
-// methodID is the `method` label for a request: the request method
-// when the router can route it, and the unmatched sentinel otherwise.
-//
-// net/http accepts any RFC 9110 token as a method and hands it
-// through verbatim, so the raw method is client-chosen bytes and
-// bounds the label at nothing — the same unauthenticated
-// series-minting the handler label carried, reached through a second
-// dimension. What bounds it is the set chi's router will match a
-// route for: its methodMap, which is unexported, so it is restated
-// here against the net/http constants it is built from. A token
-// outside that set can only ever produce chi's 405, so folding every
-// one of them onto a single series loses no information a scrape
-// could have used, while the nine methods that can reach a handler
-// stay distinguishable.
-//
-// chi.RegisterMethod would extend the router's set at runtime; this
-// service never calls it, and a caller that started to would have to
-// extend this switch with it.
-func methodID(method string) string {
-	switch method {
-	case http.MethodConnect,
-		http.MethodDelete,
-		http.MethodGet,
-		http.MethodHead,
-		http.MethodOptions,
-		http.MethodPatch,
-		http.MethodPost,
-		http.MethodPut,
-		http.MethodTrace:
-		return method
-	default:
-		return unmatchedMethod
-	}
-}
-
-// boundedLabelRecorder wraps a go-http-metrics recorder and replaces
-// the request-controlled labels on every observation with bounded
-// ones: the handler id becomes the request's route pattern, and the
-// method becomes one the router can route.
-//
-// This is the seam that makes the pattern usable at all. The metrics
-// middleware is global (see Server.setupGlobalMiddleware), so it is
-// entered before chi has matched anything, and go-http-metrics fixes
-// its handler id up front — passing the pattern in as that id is not
-// possible, and leaving the id empty makes the library substitute the
-// raw URL path, which is the defect. What the library does hand over
-// is the request context, unchanged, on each recorder call; that
-// context carries the same *chi.Context pointer routing mutates in
-// place, and the duration and size calls happen after the wrapped
-// handler has returned. Reading the pattern there is what the access
-// log already does in accessLogURL.
-//
-// Recording after the whole chain returns is also what makes this
-// hold for requests the route-level receiver rate limiter rejects.
-// Those never reach a handler, but chi has already matched the route
-// by the time the limiter runs, so their 429s land on the pattern
-// like any other response.
-type boundedLabelRecorder struct {
-	inner httpmetrics.Recorder
-}
-
-func (r boundedLabelRecorder) ObserveHTTPRequestDuration(
-	ctx context.Context,
-	props httpmetrics.HTTPReqProperties,
-	duration time.Duration,
-) {
-	props.ID = routePatternID(ctx)
-	props.Method = methodID(props.Method)
-	r.inner.ObserveHTTPRequestDuration(ctx, props, duration)
-}
-
-func (r boundedLabelRecorder) ObserveHTTPResponseSize(
-	ctx context.Context,
-	props httpmetrics.HTTPReqProperties,
-	sizeBytes int64,
-) {
-	props.ID = routePatternID(ctx)
-	props.Method = methodID(props.Method)
-	r.inner.ObserveHTTPResponseSize(ctx, props, sizeBytes)
-}
-
-func (r boundedLabelRecorder) AddInflightRequests(
-	ctx context.Context,
-	props httpmetrics.HTTPProperties,
-	quantity int,
-) {
-	props.ID = inflightHandler
-	r.inner.AddInflightRequests(ctx, props, quantity)
-}
-
-var _ httpmetrics.Recorder = boundedLabelRecorder{}
-
-// Metrics returns middleware that records Prometheus HTTP metrics on
-// the default registry, which is the one the /metrics route gathers.
-func (s *Middleware) Metrics() func(http.Handler) http.Handler {
-	return metricsMiddleware(
-		prommetrics.NewRecorder(prommetrics.Config{}),
-	)
-}
-
-// metricsMiddleware builds the recording middleware against a given
-// recorder, so tests can gather from a registry of their own instead
-// of the process-wide default.
-func metricsMiddleware(
-	rec httpmetrics.Recorder,
-) func(http.Handler) http.Handler {
-	mdlw := ghmm.New(ghmm.Config{
-		Recorder: boundedLabelRecorder{inner: rec},
-	})
-
-	return func(next http.Handler) http.Handler {
-		// The handler id is unmatchedRoute rather than "" so that
-		// the client-chosen URL path never enters the metrics
-		// pipeline at all: an empty id is the library's signal to
-		// substitute it. boundedLabelRecorder overwrites this value
-		// on every observation, so it is reachable only if that
-		// decorator is removed — in which case the metrics collapse
-		// to one series instead of leaking again.
-		return std.Handler(unmatchedRoute, mdlw, next)
-	}
-}

internal/middleware/metrics_method_test.go

@@ -1,309 +0,0 @@
-package middleware_test
-
-import (
-	"net/http"
-	"net/http/httptest"
-	"strings"
-	"testing"
-
-	"github.com/google/uuid"
-	"github.com/prometheus/client_golang/prometheus"
-	"github.com/prometheus/client_golang/prometheus/promhttp"
-	dto "github.com/prometheus/client_model/go"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/middleware"
-)
-
-const (
-	// metricsProbeMethods is how many distinct invented method tokens
-	// each cardinality assertion drives. The measurement on the issue
-	// took 300 tokens from 106 exposition lines to 7,631 — about 25
-	// permanent lines per token, never reclaimed — so a probe of this
-	// size puts a regression thousands of lines over the bound rather
-	// than leaving it to a rounding argument.
-	metricsProbeMethods = 300
-
-	// probeMethodLen is how many characters each invented method
-	// token carries, matching the 12 the issue measured with.
-	probeMethodLen = 12
-
-	// methodLabel is the label these tests are about.
-	methodLabel = "method"
-)
-
-// realMethods is the positive control's domain: the methods chi's
-// router can match a route for, every one of which a client
-// legitimately sends and every one of which must keep a series of its
-// own. Bounding the label by collapsing these into one bucket would
-// destroy the metric it is meant to protect.
-func realMethods() []string {
-	return []string{
-		http.MethodConnect, http.MethodDelete, http.MethodGet,
-		http.MethodHead, http.MethodOptions, http.MethodPatch,
-		http.MethodPost, http.MethodPut, http.MethodTrace,
-	}
-}
-
-// methodProbePath returns the one receiver path a method probe
-// targets. Holding the path fixed leaves the method as the only
-// dimension varying, so any series growth a probe produces is the
-// method label's and nothing else's.
-func methodProbePath() string {
-	return "/webhook/" + uuid.NewString()
-}
-
-// inventedMethods returns n distinct RFC 9110 method tokens that no
-// router will ever match: uppercase hex from a fresh UUID, which is
-// both the shape and the length an unauthenticated flood would send.
-// net/http accepts any token as a method, so every one of these
-// reaches the metrics pipeline exactly as a real method does.
-func inventedMethods(n int) []string {
-	methods := make([]string, 0, n)
-
-	for range n {
-		token := strings.ToUpper(
-			strings.ReplaceAll(uuid.NewString(), "-", ""),
-		)
-		methods = append(methods, token[:probeMethodLen])
-	}
-
-	return methods
-}
-
-// driveMethods sends one request per supplied method to a single
-// fixed path.
-func driveMethods(
-	t *testing.T,
-	h http.Handler,
-	path string,
-	methods []string,
-) map[int]int {
-	t.Helper()
-
-	probes := make([]probe, 0, len(methods))
-
-	for _, m := range methods {
-		probes = append(probes, probe{method: m, path: path})
-	}
-
-	return drive(t, h, probes)
-}
-
-// methodLabels returns the set of distinct `method` values across
-// every gathered series that carries the label at all. The inflight
-// gauge does not carry it, and so contributes nothing rather than an
-// empty-string member.
-func methodLabels(families []*dto.MetricFamily) map[string]struct{} {
-	seen := make(map[string]struct{})
-
-	for _, fam := range families {
-		for _, m := range fam.GetMetric() {
-			for _, pair := range m.GetLabel() {
-				if pair.GetName() == methodLabel {
-					seen[pair.GetValue()] = struct{}{}
-				}
-			}
-		}
-	}
-
-	return seen
-}
-
-// scrapeLines renders the registry through the same promhttp handler
-// /metrics is mounted on and counts the sample lines it produced.
-//
-// This is the quantity the issue measured and the one a Prometheus
-// server pays for on every scrape: one histogram label set is a
-// single gathered series but around 25 lines of exposition, which is
-// why 300 method tokens cost thousands of lines rather than hundreds.
-func scrapeLines(t *testing.T, reg *prometheus.Registry) int {
-	t.Helper()
-
-	h := promhttp.HandlerFor(reg, promhttp.HandlerOpts{})
-	req := httptest.NewRequestWithContext(
-		t.Context(), http.MethodGet, "/metrics", nil,
-	)
-	w := httptest.NewRecorder()
-	h.ServeHTTP(w, req)
-	require.Equal(t, http.StatusOK, w.Code)
-
-	lines := 0
-
-	for line := range strings.SplitSeq(w.Body.String(), "\n") {
-		if line == "" || strings.HasPrefix(line, "#") {
-			continue
-		}
-
-		lines++
-	}
-
-	return lines
-}
-
-// TestMetrics_MethodSentinelIsTheRouteSentinel pins the convention
-// rather than the mechanism. An unroutable method and an unmatched
-// path are the same fact — a client-chosen token matching nothing
-// this service registers — so they carry one spelling. Two spellings
-// would read in a scrape as two different unmatched states.
-func TestMetrics_MethodSentinelIsTheRouteSentinel(t *testing.T) {
-	t.Parallel()
-
-	assert.Equal(
-		t,
-		middleware.UnmatchedRouteConst,
-		middleware.UnmatchedMethodConst,
-		"the unmatched sentinel must have exactly one spelling",
-	)
-}
-
-// TestMetrics_InventedMethodsMintOneLabelSet is the direct assertion
-// the issue asks for: N requests carrying N distinct invented method
-// tokens must produce exactly ONE method label. Before the fix this
-// produced N of them, on an unauthenticated route with no rate
-// limiter.
-func TestMetrics_InventedMethodsMintOneLabelSet(t *testing.T) {
-	t.Parallel()
-
-	h, reg := metricsTestRouter(t, generousReceiverLimit)
-
-	methods := inventedMethods(metricsProbeMethods)
-
-	codes := driveMethods(t, h, methodProbePath(), methods)
-	require.Equal(
-		t, metricsProbeMethods, codes[http.StatusMethodNotAllowed],
-		"every invented token should have been unroutable",
-	)
-
-	labels := methodLabels(gatherMetrics(t, reg))
-
-	// Asserted on the count rather than on the set, so that a
-	// regression reports one number instead of dumping every token it
-	// minted.
-	distinct := len(labels)
-
-	assert.Equal(
-		t, 1, distinct,
-		"invented methods must collapse onto one label",
-	)
-	assert.Contains(
-		t, keys(labels), middleware.UnmatchedMethodConst,
-		"that one label must be the unmatched sentinel",
-	)
-
-	// The scrape must not republish the tokens it was driven with
-	// either: a label that merely looks bounded while still echoing
-	// client bytes is the same defect wearing a different name.
-	echoed := 0
-
-	for _, m := range methods {
-		for label := range labels {
-			if strings.Contains(label, m) {
-				echoed++
-			}
-		}
-	}
-
-	assert.Equal(
-		t, 0, echoed,
-		"invented method tokens reached the metrics labels",
-	)
-}
-
-// TestMetrics_MethodSeriesCountIsFlatUnderAFlood reproduces the
-// measurement on the issue in miniature: scrape, drive several
-// hundred distinct method tokens, scrape again, and require the
-// second scrape to be no larger than the first. The first batch
-// establishes every label set the route can produce; a flood five
-// times its size must land on exactly those.
-func TestMetrics_MethodSeriesCountIsFlatUnderAFlood(t *testing.T) {
-	t.Parallel()
-
-	h, reg := metricsTestRouter(t, generousReceiverLimit)
-
-	path := methodProbePath()
-
-	driveMethods(t, h, path, inventedMethods(metricsProbeMethods))
-	seededSeries := seriesCount(gatherMetrics(t, reg))
-	seededLines := scrapeLines(t, reg)
-
-	driveMethods(t, h, path, inventedMethods(metricsProbeMethods*4))
-	floodedSeries := seriesCount(gatherMetrics(t, reg))
-	floodedLines := scrapeLines(t, reg)
-
-	t.Logf(
-		"after %d invented methods: %d series, %d lines; "+
-			"after %d more: %d series, %d lines",
-		metricsProbeMethods, seededSeries, seededLines,
-		metricsProbeMethods*4, floodedSeries, floodedLines,
-	)
-
-	assert.Equal(
-		t, seededSeries, floodedSeries,
-		"a flood of invented methods must not mint series",
-	)
-	assert.Equal(
-		t, seededLines, floodedLines,
-		"a flood of invented methods must not grow the scrape",
-	)
-}
-
-// TestMetrics_RealMethodsStayDistinct is the positive control. The
-// bound is worth nothing if it is bought by flattening the metric:
-// every method the router can route must still carry a series of its
-// own, one sample each, under the route pattern it was sent to.
-func TestMetrics_RealMethodsStayDistinct(t *testing.T) {
-	t.Parallel()
-
-	h, reg := metricsTestRouter(t, generousReceiverLimit)
-
-	methods := realMethods()
-
-	codes := driveMethods(t, h, methodProbePath(), methods)
-	require.Equal(
-		t, len(methods), codes[http.StatusNotFound],
-		"every real method should have reached the receiver",
-	)
-
-	families := gatherMetrics(t, reg)
-
-	want := make(map[string]struct{}, len(methods))
-	for _, m := range methods {
-		want[m] = struct{}{}
-	}
-
-	assert.Equal(
-		t, want, methodLabels(families),
-		"real methods must remain distinguishable",
-	)
-
-	// Appearing somewhere in the scrape is not enough: each method
-	// must own its duration series, holding the one sample it sent.
-	observed := 0
-
-	for _, fam := range families {
-		if !strings.HasSuffix(fam.GetName(), "request_duration_seconds") {
-			continue
-		}
-
-		for _, m := range fam.GetMetric() {
-			observed++
-
-			assert.Equal(
-				t, receiverRoutePattern,
-				labelValue(m, "handler"),
-			)
-			assert.Equal(
-				t, uint64(1),
-				m.GetHistogram().GetSampleCount(),
-				"method %q shares a series",
-				labelValue(m, methodLabel),
-			)
-		}
-	}
-
-	assert.Equal(
-		t, len(methods), observed,
-		"one duration series per routable method",
-	)
-}

internal/middleware/metrics_test.go

@@ -1,457 +0,0 @@
-package middleware_test
-
-import (
-	"log/slog"
-	"net/http"
-	"net/http/httptest"
-	"strings"
-	"testing"
-
-	"github.com/go-chi/chi"
-	"github.com/google/uuid"
-	"github.com/prometheus/client_golang/prometheus"
-	dto "github.com/prometheus/client_model/go"
-	prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/config"
-	"sneak.berlin/go/webhooker/internal/middleware"
-)
-
-const (
-	// metricsProbePaths is how many distinct receiver paths each
-	// cardinality assertion drives. The defect these tests pin cost
-	// roughly 26 permanent series per distinct path, so a couple of
-	// hundred puts a regression thousands of series over the bound
-	// rather than leaving it to a rounding argument.
-	metricsProbePaths = 250
-
-	// receiverRoutePattern is the one handler label every receiver
-	// request must produce, however the client varies the path.
-	receiverRoutePattern = "/webhook/{uuid}"
-
-	// okRoute is a static route used to pin that the response-writer
-	// interceptor still reports status and size after the handler id
-	// stopped coming from the URL.
-	okRoute = "/ok"
-
-	// okBody is what okRoute writes, so the recorded response size is
-	// a number the test knows.
-	okBody = "ok"
-
-	// generousReceiverLimit is a per-entrypoint receiver limit high
-	// enough that no probe in this file trips the limiter unless it
-	// means to.
-	generousReceiverLimit = 100000
-
-	// tightReceiverLimit forces the receiver's aggregate limiter to
-	// reject: the aggregate ceiling is ten times this, so a probe of
-	// metricsProbePaths requests spends it many times over.
-	tightReceiverLimit = 1
-)
-
-// metricsTestRouter builds a router whose middleware ordering mirrors
-// the real server's: the metrics recorder is GLOBAL, installed by
-// Server.setupGlobalMiddleware before chi has matched anything, and
-// the receiver rate limiter is ROUTE-LEVEL, installed by
-// Server.setupWebhookRoutes inside it. That ordering is the whole
-// defect, so a test that flattens it would prove nothing.
-//
-// The recorder writes to a registry of the test's own rather than the
-// process-wide default one, so each test observes only its own
-// traffic.
-func metricsTestRouter(
-	t *testing.T,
-	receiverLimit int,
-) (http.Handler, *prometheus.Registry) {
-	t.Helper()
-
-	log := slog.New(slog.DiscardHandler)
-	cfg := &config.Config{
-		Environment:       "prod",
-		ReceiverRateLimit: receiverLimit,
-	}
-	m := middleware.NewForTest(
-		log, cfg, newTestSessionManager(cfg, log, nil),
-	)
-
-	reg := prometheus.NewRegistry()
-	rec := prommetrics.NewRecorder(prommetrics.Config{Registry: reg})
-
-	r := chi.NewRouter()
-	r.Use(middleware.MetricsMiddlewareForTest(rec))
-
-	r.Get(okRoute, func(w http.ResponseWriter, _ *http.Request) {
-		_, _ = w.Write([]byte(okBody))
-	})
-
-	// The real receiver answers 404 for a UUID naming no stored
-	// entrypoint, which is what every invented path here is.
-	r.With(m.ReceiverRateLimit()).HandleFunc(
-		receiverRoutePattern,
-		func(w http.ResponseWriter, _ *http.Request) {
-			w.WriteHeader(http.StatusNotFound)
-		},
-	)
-
-	return r, reg
-}
-
-// probe is one request a cardinality assertion sends. Both label
-// dimensions that have leaked are request-controlled — the path and
-// the method — so both vary here and one driver sends them.
-type probe struct {
-	method string
-	path   string
-}
-
-// drive sends every probe and returns how many responses carried each
-// status code.
-func drive(t *testing.T, h http.Handler, probes []probe) map[int]int {
-	t.Helper()
-
-	codes := make(map[int]int)
-
-	for _, p := range probes {
-		req := httptest.NewRequestWithContext(
-			t.Context(), p.method, p.path, nil,
-		)
-		w := httptest.NewRecorder()
-		h.ServeHTTP(w, req)
-		codes[w.Code]++
-	}
-
-	return codes
-}
-
-// drivePaths sends one POST per supplied path.
-func drivePaths(
-	t *testing.T,
-	h http.Handler,
-	paths []string,
-) map[int]int {
-	t.Helper()
-
-	probes := make([]probe, 0, len(paths))
-
-	for _, p := range paths {
-		probes = append(
-			probes, probe{method: http.MethodPost, path: p},
-		)
-	}
-
-	return drive(t, h, probes)
-}
-
-// receiverPaths returns n distinct /webhook/ paths, each naming a
-// fresh UUID exactly as an unauthenticated flood would.
-func receiverPaths(n int) []string {
-	paths := make([]string, 0, n)
-
-	for range n {
-		paths = append(paths, "/webhook/"+uuid.NewString())
-	}
-
-	return paths
-}
-
-// gatherMetrics returns the registry's current families, failing the
-// test if gathering does.
-func gatherMetrics(
-	t *testing.T,
-	reg *prometheus.Registry,
-) []*dto.MetricFamily {
-	t.Helper()
-
-	families, err := reg.Gather()
-	require.NoError(t, err)
-
-	return families
-}
-
-// labelValue returns the named label from a gathered metric.
-func labelValue(m *dto.Metric, name string) string {
-	for _, pair := range m.GetLabel() {
-		if pair.GetName() == name {
-			return pair.GetValue()
-		}
-	}
-
-	return ""
-}
-
-// handlerLabels returns the set of distinct `handler` label values
-// across every gathered series.
-func handlerLabels(families []*dto.MetricFamily) map[string]struct{} {
-	seen := make(map[string]struct{})
-
-	for _, fam := range families {
-		for _, m := range fam.GetMetric() {
-			seen[labelValue(m, "handler")] = struct{}{}
-		}
-	}
-
-	return seen
-}
-
-// seriesCount is the number of distinct label sets held across every
-// family: the quantity that grew without bound and was never
-// reclaimed.
-func seriesCount(families []*dto.MetricFamily) int {
-	total := 0
-
-	for _, fam := range families {
-		total += len(fam.GetMetric())
-	}
-
-	return total
-}
-
-// keys returns the members of a set, for assertion messages.
-func keys(set map[string]struct{}) []string {
-	out := make([]string, 0, len(set))
-
-	for k := range set {
-		out = append(out, k)
-	}
-
-	return out
-}
-
-// TestMetrics_DistinctReceiverPathsMintOneLabelSet is the direct
-// assertion the issue asks for: N requests to N distinct
-// /webhook/<uuid> paths must produce exactly ONE handler label, the
-// route pattern. Before the fix this produced N of them.
-func TestMetrics_DistinctReceiverPathsMintOneLabelSet(t *testing.T) {
-	t.Parallel()
-
-	h, reg := metricsTestRouter(t, generousReceiverLimit)
-
-	paths := receiverPaths(metricsProbePaths)
-	codes := drivePaths(t, h, paths)
-	require.Equal(
-		t, metricsProbePaths, codes[http.StatusNotFound],
-		"every invented UUID should have reached the receiver",
-	)
-
-	families := gatherMetrics(t, reg)
-	labels := handlerLabels(families)
-
-	assert.Equal(
-		t,
-		map[string]struct{}{
-			receiverRoutePattern:            {},
-			middleware.InflightHandlerConst: {},
-		},
-		labels,
-		"receiver traffic must collapse onto the route pattern",
-	)
-
-	// The scrape must not republish the UUIDs it was driven with.
-	// They are the receiver's only credential.
-	for _, p := range paths {
-		id := strings.TrimPrefix(p, "/webhook/")
-		for label := range labels {
-			assert.NotContains(
-				t, label, id,
-				"an entrypoint UUID reached a metrics label",
-			)
-		}
-	}
-}
-
-// TestMetrics_SeriesCountIsFlatUnderAFlood pins the property the
-// issue measured against a live instance: driving thousands more
-// distinct paths must not add series. The first batch establishes
-// every label set the route can produce; the second must land on
-// exactly those.
-func TestMetrics_SeriesCountIsFlatUnderAFlood(t *testing.T) {
-	t.Parallel()
-
-	h, reg := metricsTestRouter(t, generousReceiverLimit)
-
-	drivePaths(t, h, receiverPaths(metricsProbePaths))
-	before := seriesCount(gatherMetrics(t, reg))
-
-	drivePaths(t, h, receiverPaths(metricsProbePaths*4))
-	after := seriesCount(gatherMetrics(t, reg))
-
-	assert.Equal(
-		t, before, after,
-		"a flood of distinct paths must not mint series",
-	)
-}
-
-// TestMetrics_RateLimitedRequestsCarryTheRoutePattern covers the
-// majority case: most of the leaked series were 429s. Those requests
-// never reach a handler, so they take a different path through the
-// stack — but chi has already matched the route by the time the
-// route-level limiter rejects them, and the recording happens after
-// the whole chain returns, so they must land on the pattern too.
-func TestMetrics_RateLimitedRequestsCarryTheRoutePattern(t *testing.T) {
-	t.Parallel()
-
-	h, reg := metricsTestRouter(t, tightReceiverLimit)
-
-	codes := drivePaths(t, h, receiverPaths(metricsProbePaths))
-	require.Positive(
-		t, codes[http.StatusTooManyRequests],
-		"the probe must actually exhaust the aggregate limiter",
-	)
-
-	families := gatherMetrics(t, reg)
-
-	assert.Equal(
-		t,
-		map[string]struct{}{
-			receiverRoutePattern:            {},
-			middleware.InflightHandlerConst: {},
-		},
-		handlerLabels(families),
-		"rejected requests must collapse onto the route pattern",
-	)
-
-	rejected := 0
-
-	for _, fam := range families {
-		for _, m := range fam.GetMetric() {
-			if labelValue(m, "code") != "429" {
-				continue
-			}
-
-			rejected++
-
-			assert.Equal(
-				t, receiverRoutePattern,
-				labelValue(m, "handler"),
-				"a 429 series carried a non-pattern handler",
-			)
-		}
-	}
-
-	assert.Positive(
-		t, rejected, "no 429 series was recorded at all",
-	)
-}
-
-// TestMetrics_UnmatchedPathsCollapseToTheSentinel decides and pins the
-// unmatched-route case. A path matching no route has no pattern, so
-// it carries the same fixed sentinel the access log uses. Without
-// that, an unmatched flood leaks exactly as the receiver did.
-func TestMetrics_UnmatchedPathsCollapseToTheSentinel(t *testing.T) {
-	t.Parallel()
-
-	h, reg := metricsTestRouter(t, generousReceiverLimit)
-
-	paths := make([]string, 0, metricsProbePaths)
-
-	for i := range metricsProbePaths {
-		id := uuid.NewString()
-
-		// Two shapes: one matching no prefix at all, and one under
-		// the receiver prefix but with a segment count the pattern
-		// cannot match.
-		if i%2 == 0 {
-			paths = append(paths, "/"+id)
-		} else {
-			paths = append(paths, "/webhook/"+id+"/"+id)
-		}
-	}
-
-	codes := drivePaths(t, h, paths)
-	require.Equal(
-		t, metricsProbePaths, codes[http.StatusNotFound],
-		"every probe path should have gone unmatched",
-	)
-
-	labels := handlerLabels(gatherMetrics(t, reg))
-
-	assert.Equal(
-		t,
-		map[string]struct{}{
-			middleware.UnmatchedRouteConst:  {},
-			middleware.InflightHandlerConst: {},
-		},
-		labels,
-		"unmatched paths must collapse onto one sentinel, got %v",
-		keys(labels),
-	)
-}
-
-// TestMetrics_InflightGaugeIsAggregateAndBalanced pins the one metric
-// that cannot carry a pattern. It is incremented before routing and
-// decremented after, so it gets a fixed label -- and the two calls
-// must therefore agree, leaving the gauge at zero once the traffic
-// has drained rather than stuck above it.
-func TestMetrics_InflightGaugeIsAggregateAndBalanced(t *testing.T) {
-	t.Parallel()
-
-	h, reg := metricsTestRouter(t, generousReceiverLimit)
-
-	drivePaths(t, h, receiverPaths(metricsProbePaths))
-
-	var inflight []*dto.Metric
-
-	for _, fam := range gatherMetrics(t, reg) {
-		if strings.HasSuffix(fam.GetName(), "requests_inflight") {
-			inflight = fam.GetMetric()
-		}
-	}
-
-	require.Len(
-		t, inflight, 1,
-		"the inflight gauge must hold exactly one series",
-	)
-	assert.Equal(
-		t, middleware.InflightHandlerConst,
-		labelValue(inflight[0], "handler"),
-	)
-	assert.InDelta(
-		t, 0.0, inflight[0].GetGauge().GetValue(), 0.0,
-		"the gauge must balance back to zero",
-	)
-}
-
-// TestMetrics_StatusAndSizeStillRecorded guards the response-writer
-// interceptor the recording middleware wraps around every request.
-// The handler label changed; what the interceptor reports must not
-// have.
-func TestMetrics_StatusAndSizeStillRecorded(t *testing.T) {
-	t.Parallel()
-
-	h, reg := metricsTestRouter(t, generousReceiverLimit)
-
-	req := httptest.NewRequestWithContext(
-		t.Context(), http.MethodGet, okRoute, nil,
-	)
-	w := httptest.NewRecorder()
-	h.ServeHTTP(w, req)
-
-	require.Equal(t, http.StatusOK, w.Code)
-	require.Equal(t, okBody, w.Body.String())
-
-	var size *dto.Metric
-
-	for _, fam := range gatherMetrics(t, reg) {
-		if !strings.HasSuffix(fam.GetName(), "response_size_bytes") {
-			continue
-		}
-
-		for _, m := range fam.GetMetric() {
-			if labelValue(m, "handler") == okRoute {
-				size = m
-			}
-		}
-	}
-
-	require.NotNil(
-		t, size, "no response size series for the static route",
-	)
-	assert.Equal(t, "200", labelValue(size, "code"))
-	assert.Equal(t, uint64(1), size.GetHistogram().GetSampleCount())
-	assert.InDelta(
-		t, float64(len(okBody)),
-		size.GetHistogram().GetSampleSum(), 0.0,
-		"the interceptor must still count written bytes",
-	)
-}

internal/middleware/middleware.go

@@ -13,6 +13,9 @@ import (
 	"github.com/go-chi/chi"
 	"github.com/go-chi/chi/middleware"
 	"github.com/go-chi/cors"
+	metrics "github.com/slok/go-http-metrics/metrics/prometheus"
+	ghmm "github.com/slok/go-http-metrics/middleware"
+	"github.com/slok/go-http-metrics/middleware/std"
 	"go.uber.org/fx"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/globals"
@@ -26,15 +29,10 @@ const (
 	// preflight response can be cached.
 	corsMaxAge = 300
 
-	// unmatchedRoute stands in for a request that matched no route
-	// pattern at all. Every byte of such a path is client-chosen, so
-	// none of it is kept.
-	//
-	// It is the access log's url field on a redirected or rejected
-	// request, and it is the metrics `handler` label on the same
-	// request; see metrics.go. Both surfaces are written once per
-	// request from a path the client picks, so both have to collapse
-	// the unmatched case into one fixed value.
+	// unmatchedRoute is logged in the access log's url field when a
+	// redirected or rejected request matched no route pattern at
+	// all. Every byte of such a path is client-chosen, so none of it
+	// is logged.
 	unmatchedRoute = "(unmatched)"
 
 	// redactedQuery stands in for the query string on the access log
@@ -440,6 +438,17 @@ func (s *Middleware) RequireAuth() func(http.Handler) http.Handler {
 	}
 }
 
+// Metrics returns middleware that records Prometheus HTTP metrics.
+func (s *Middleware) Metrics() func(http.Handler) http.Handler {
+	mdlw := ghmm.New(ghmm.Config{
+		Recorder: metrics.NewRecorder(metrics.Config{}),
+	})
+
+	return func(next http.Handler) http.Handler {
+		return std.Handler("", mdlw, next)
+	}
+}
+
 // MetricsAuth returns middleware that protects metrics endpoints
 // with basic auth.
 func (s *Middleware) MetricsAuth() func(http.Handler) http.Handler {

internal/versionscript/version_script_test.go

@@ -11,7 +11,6 @@ import (
 )
 
 const (
-	repoRoot       = "../.."
 	scriptPath     = "../../script/version"
 	makefilePath   = "../../Makefile"
 	dockerfilePath = "../../Dockerfile"
@@ -167,61 +166,6 @@ func TestMakefile_BuildComposesVersionAndExtraFlags(t *testing.T) {
 	require.Contains(t, makefile, "VERSION ?= $(shell script/version)")
 }
 
-// A caller can define VERSION as the empty string -- `make build
-// VERSION=`, or a `--build-arg VERSION=` reaching the Dockerfile's `make
-// build VERSION="$VERSION"`. script/version's own guard does not cover
-// that: the value never passes through the script. Stamping "" would
-// leave the binary reporting no version and the footer on "dev", which
-// is the defect this package exists for.
-func TestMakefile_EmptyOverrideResolvesLikeAnUnsetOne(t *testing.T) {
-	t.Parallel()
-
-	// A plain assignment would be ignored here: a command-line
-	// definition outranks it, and that is the case being corrected.
-	require.Contains(t, read(t, makefilePath), "override VERSION :=")
-
-	requireMake(t)
-
-	derived := makeVersion(t)
-	require.NotEmpty(t, derived)
-
-	require.Equal(t, derived, makeVersion(t, "VERSION="),
-		"an empty VERSION must resolve the way an unset one does")
-	require.Equal(t, "v9.9.9", makeVersion(t, "VERSION=v9.9.9"),
-		"the empty guard must not clobber a real override")
-}
-
-// makeVersion runs this repository's `version` target, which prints the
-// value `make build` would stamp, with the given command-line
-// definitions.
-func makeVersion(t *testing.T, defs ...string) string {
-	t.Helper()
-
-	//nolint:gosec // fixed argv, arguments are test constants
-	cmd := exec.CommandContext(t.Context(), "make",
-		append([]string{"--no-print-directory", "version"}, defs...)...)
-	cmd.Dir = repoRoot
-
-	// Only the command-line definitions may decide the outcome: an
-	// inherited VERSION would change what an unset one resolves to, and
-	// an inherited MAKEFLAGS carries the parent's jobserver.
-	cmd.Env = append(os.Environ(), "VERSION=", "MAKEFLAGS=", "MAKELEVEL=")
-
-	out, err := cmd.CombinedOutput()
-	require.NoError(t, err, string(out))
-
-	return strings.TrimSpace(string(out))
-}
-
-func requireMake(t *testing.T) {
-	t.Helper()
-
-	_, err := exec.LookPath("make")
-	if err != nil {
-		t.Skipf("make is not installed: %v", err)
-	}
-}
-
 // Every compile in the image goes through the build target, so the
 // static relink cannot replace the flags that carry the stamp.
 func TestDockerfile_BuildsThroughTheMakeTarget(t *testing.T) {

templates/source_logs.html

@@ -39,7 +39,7 @@
                     <div class="flex items-center gap-4">
                         {{range .Deliveries}}
                         <span class="text-xs {{if eq .Status "delivered"}}text-green-600{{else if eq .Status "failed"}}text-red-600{{else if eq .Status "retrying"}}text-yellow-600{{else}}text-gray-400{{end}}">
-                            {{.Target.DisplayName}}: {{.Status}}
+                            {{.Target.Name}}: {{.Status}}
                         </span>
                         {{end}}
                         <span class="text-xs text-gray-400">{{.CreatedAt.Format "2006-01-02 15:04:05"}}</span>
@@ -74,7 +74,7 @@
                             <div class="py-2" x-data="{ attempts: false }">
                                 <div class="flex items-center justify-between cursor-pointer" @click="attempts = !attempts">
                                     <div class="flex items-center gap-3">
-                                        <span class="text-sm text-gray-700">{{.Target.DisplayName}}</span>
+                                        <span class="text-sm text-gray-700">{{.Target.Name}}</span>
                                         <span class="text-xs {{if eq .Status "delivered"}}text-green-600{{else if eq .Status "failed"}}text-red-600{{else if eq .Status "retrying"}}text-yellow-600{{else}}text-gray-400{{end}}">{{.Status}}</span>
                                     </div>
                                     <div class="flex items-center gap-3">