Add a Download button that exports a database target's archive as gzipped JSON (closes #374)

Each database target on the webhook page links to
/hook/ID/targets/TARGETID/download, which streams the target's archive
as archive-WEBHOOK-TARGET-YYYYMMDDTHHMMSSZ.json.gz: the webhook, the
target, exported_at, and archived_events, one object per row keyed by
column, a body that is not valid UTF-8 in base64 with body_encoding.
The export reads the rows through one cursor on its own connection
inside a read-only transaction: one snapshot, and no write lock. A
download runs for as long as the client keeps reading, and one that
fails after it has started aborts the connection. The handler holds the
rename lock only while it reads the names and opens the file. A missing
archive exports empty and is not created.

Model: opus-5-5

README.md

@@ -139,7 +139,7 @@ TTY detection, and security headers are always applied.
 | `METRICS_USERNAME`      | Basic auth username for `/metrics`. Must be set together with `METRICS_PASSWORD`; one without the other fails startup | `""`     |
 | `METRICS_PASSWORD`      | Basic auth password for `/metrics`. Must be set together with `METRICS_USERNAME`; one without the other fails startup | `""`     |
 | `SENTRY_DSN`            | Sentry error reporting DSN. Unset leaves error reporting off; a value the Sentry SDK cannot parse fails startup rather than serving with reporting silently off | `""`     |
-| `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive). A value that does not parse, or is zero or negative, fails startup | `1h` |
+| `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive) | `1h` |
 | `SESSION_IDLE_TIMEOUT`  | Idle session timeout (Go duration)  | `24h`    |
 | `RECEIVER_RATE_LIMIT`   | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` |
 | `TRUSTED_PROXIES`       | CIDRs whose forwarded headers are trusted. A set value replaces the default. If any client can reach webhooker, or the proxy in front of it, from an RFC 1918 source address, set it to the proxy's address alone. See [Trusted proxies](#trusted-proxies) | `10.0.0.0/8,172.16.0.0/12,192.168.0.0/16` (RFC 1918) |
@@ -557,8 +557,8 @@ If it is lost, run `webhooker resetpw admin` on a stopped deployment.
 ```
 
 It is a banner rather than a log line because that is the only time it
-is ever shown: as one `INFO` record it would sit among the records fx
-writes as each start hook runs, and under `docker run -d`
+is ever shown: as one `INFO` record it sat among the roughly 45 fx
+`PROVIDE`/`RUN`/`HOOK` lines a boot writes, and under `docker run -d`
 it is one line in a log subject to rotation. The database stores only
 its Argon2id hash. There is no second account and no forgot-password
 flow, so the banner and the reset command below are the only two ways
@@ -628,8 +628,7 @@ Changing a password you still know needs none of this — use
 
 `DEBUG=true` lowers the log level to `DEBUG`, which turns on every
 statement GORM runs, the two by-design lookup misses on the
-unauthenticated routes, the rate limiter's own rejections, and fx's
-records of building the dependency graph at startup. It is
+unauthenticated routes, and the rate limiter's own rejections. It is
 meant to be safe to turn on while diagnosing a live service and safe to
 paste the output of into a bug report.
 
@@ -2661,20 +2660,16 @@ read as more than it is:
   that type on a specific webhook, and each line it writes is bounded
   per event by the 1 MB receiver body cap. Adding one is a decision to
   spend log volume on that webhook's payloads.
-- **The Go runtime**, which does not go through `internal/logger`. The
-  runtime writes an unrecovered panic or a fatal error itself, as plain
-  text on standard error, and that output cannot be redirected. A panic
-  in a background worker rather than in a request handler is the case
-  that reaches it, since nothing recovers those. It carries no
-  client-chosen value at a client-chosen length: the service's own
-  `panic` calls are invariant guards over constants and over
-  `crypto/rand`, apart from the one that hands `http.ErrAbortHandler`
-  back to `net/http`, described below.
-- **A failure before fx's logger is built**, such as an invalid
-  configuration value. fx's logger takes the configuration, so when
-  that fails fx's own console logger still prints the failure as plain
-  text on standard error. Its values come from the operator's
-  environment, not from a client.
+- **Two writers that do not go through `internal/logger` at all**, both
+  on standard error. `fx` prints the dependency graph and the lifecycle
+  hooks through its default console logger at startup and shutdown —
+  nothing calls `fx.WithLogger`, and `fx.New` builds that logger over
+  `os.Stderr`. The Go runtime writes a panic or a fatal error itself; a
+  panic in a background worker rather than in a request handler is the
+  case that reaches it, since nothing recovers those. Neither carries a
+  client-chosen value at a client-chosen length: the five `panic` calls
+  in this service are invariant guards over constants and over
+  `crypto/rand`.
 - **`net/http`'s own faults**, which are _not_ a separate writer.
   `internal/server/http.go` builds its server with a nil `ErrorLog`, so
   `net/http` falls back to the `log` package's default logger — and
@@ -2966,8 +2961,7 @@ webhooker/
 │   ├── resetpw/
 │   │   └── resetpw.go         # `webhooker resetpw`: set an account's password, stopped deployments only
 │   ├── config/
-│   │   ├── config.go          # Configuration loading from environment variables
-│   │   └── testing.go         # ClearEnvForTest: an empty environment for one test
+│   │   └── config.go          # Configuration loading from environment variables
 │   ├── database/
 │   │   ├── base_model.go      # BaseModel with UUID primary keys
 │   │   ├── database.go        # GORM connection, migrations, admin seed
@@ -3027,7 +3021,7 @@ webhooker/
 │   ├── lifecycle/
 │   │   └── lifecycle.go       # Shared stop-hook waiter, bounded by the stop context
 │   ├── logger/
-│   │   └── logger.go          # slog setup with TTY detection; fx's event logger
+│   │   └── logger.go          # slog setup with TTY detection
 │   ├── metrics/
 │   │   └── metrics.go         # Delivery Prometheus collectors, labelled by target type
 │   ├── middleware/

cmd/webhooker/main.go

@@ -8,7 +8,6 @@ import (
 	"time"
 
 	"go.uber.org/fx"
-	"go.uber.org/fx/fxevent"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/datadir"
@@ -169,19 +168,6 @@ func run(stderr io.Writer) int {
 func newApp() *fx.App {
 	return fx.New(
 		fx.StopTimeout(stopTimeout),
-		// fx's own events go through the service's logger, not fx's
-		// console logger on standard error. The exception is a failure
-		// before this logger is built, such as an invalid configuration
-		// value, which fx's console logger still prints there. fx holds
-		// its events back until this logger is built and then replays
-		// them, so it takes the configuration, which sets the level
-		// DEBUG=true asks for: without it the replay would run at INFO
-		// and drop every record of how the graph was built.
-		fx.WithLogger(
-			func(l *logger.Logger, _ *config.Config) fxevent.Logger {
-				return logger.NewFxLogger(l.Get())
-			},
-		),
 		fx.Provide(
 			globals.New,
 			logger.New,

cmd/webhooker/main_test.go

@@ -2,19 +2,12 @@ package main
 
 import (
 	"bytes"
-	"encoding/json"
-	"io"
-	"log/slog"
-	"net"
-	"os"
-	"strconv"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/datadir"
 	"sneak.berlin/go/webhooker/internal/resetpw"
 	"sneak.berlin/go/webhooker/internal/server"
@@ -37,7 +30,6 @@ const dockerStopGrace = 10 * time.Second
 // fx.New applies options before it executes invokes, so the timeout
 // is set whether or not the graph itself can be constructed here.
 func TestNewApp_StopTimeout(t *testing.T) {
-	config.ClearEnvForTest(t)
 	t.Setenv("DATA_DIR", t.TempDir())
 
 	got := newApp().StopTimeout()
@@ -46,100 +38,6 @@ func TestNewApp_StopTimeout(t *testing.T) {
 	require.Less(t, got, dockerStopGrace)
 }
 
-// freePort returns a loopback TCP port that was free a moment ago, by
-// taking one and releasing it.
-func freePort(t *testing.T) int {
-	t.Helper()
-
-	var listenCfg net.ListenConfig
-
-	l, err := listenCfg.Listen(t.Context(), "tcp", "127.0.0.1:0")
-	require.NoError(t, err)
-
-	addr, ok := l.Addr().(*net.TCPAddr)
-	require.True(t, ok, "listener is not TCP")
-	require.NoError(t, l.Close())
-
-	return addr.Port
-}
-
-// TestNewApp_SendsFxEventsToTheLogger starts and stops the app main
-// runs, with DEBUG=true, and reads back what reached the service's
-// logger. fx's own events must arrive there as structured records:
-// the start at INFO, and at DEBUG the records of how the graph was
-// built.
-//
-// fx holds its events back until its logger is built and then replays
-// them all at once, so the earliest of them arriving shows the replay
-// ran at DEBUG: that globals.New was provided, which fx records before
-// anything is built, and the run of logger.New, which happens before
-// the configuration sets the level.
-func TestNewApp_SendsFxEventsToTheLogger(t *testing.T) {
-	config.ClearEnvForTest(t)
-	t.Setenv("DATA_DIR", t.TempDir())
-	t.Setenv("PORT", strconv.Itoa(freePort(t)))
-	t.Setenv("DEBUG", "true")
-
-	// internal/logger writes to whatever os.Stdout is when it builds
-	// its handler. A file is not a terminal, so that handler is the
-	// JSON one the service uses in production.
-	out, err := os.CreateTemp(t.TempDir(), "stdout")
-	require.NoError(t, err)
-
-	stdout := os.Stdout
-	os.Stdout = out
-
-	t.Cleanup(func() {
-		os.Stdout = stdout
-		_ = out.Close()
-	})
-
-	app := newApp()
-	require.NoError(t, app.Start(t.Context()))
-	require.NoError(t, app.Stop(t.Context()))
-
-	_, err = out.Seek(0, io.SeekStart)
-	require.NoError(t, err)
-
-	written, err := io.ReadAll(out)
-	require.NoError(t, err)
-
-	type record struct {
-		Level       string `json:"level"`
-		Msg         string `json:"msg"`
-		Name        string `json:"name"`
-		Constructor string `json:"constructor"`
-	}
-
-	var records []record
-
-	for line := range strings.Lines(string(written)) {
-		var r record
-
-		// The first-boot banner is plain text, not a record.
-		if json.Unmarshal([]byte(line), &r) == nil {
-			records = append(records, r)
-		}
-	}
-
-	const pkg = "sneak.berlin/go/webhooker/internal/"
-
-	info := slog.LevelInfo.String()
-	debug := slog.LevelDebug.String()
-
-	assert.Contains(t, records, record{Level: info, Msg: "started"})
-	assert.Contains(t, records, record{
-		Level: debug, Msg: "provided", Constructor: pkg + "globals.New()",
-	})
-	assert.Contains(t, records, record{
-		Level: debug, Msg: "run", Name: pkg + "logger.New()",
-	})
-	assert.Contains(t, records, record{Level: debug, Msg: "invoking"})
-	assert.Contains(t, records, record{
-		Level: debug, Msg: "initialized custom fxevent.Logger",
-	})
-}
-
 // TestRunRefusesLockedDataDir pins what an operator's second start
 // does. The entry point must refuse before it builds the fx graph —
 // nothing may open a database in a DATA_DIR another process holds —

go.mod

@@ -20,7 +20,7 @@ require (
 	github.com/prometheus/client_model v0.5.0
 	github.com/slok/go-http-metrics v0.11.0
 	github.com/stretchr/testify v1.11.1
-	go.uber.org/fx v1.24.0
+	go.uber.org/fx v1.20.1
 	golang.org/x/crypto v0.38.0
 	gopkg.in/yaml.v3 v3.0.1
 	gorm.io/driver/sqlite v1.5.4
@@ -42,6 +42,7 @@ require (
 	github.com/jinzhu/now v1.1.5 // indirect
 	github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 // indirect
 	github.com/klauspost/cpuid/v2 v2.2.10 // indirect
+	github.com/kr/text v0.2.0 // indirect
 	github.com/mattn/go-isatty v0.0.20 // indirect
 	github.com/mattn/go-sqlite3 v1.14.17 // indirect
 	github.com/matttproud/golang_protobuf_extensions/v2 v2.0.0 // indirect
@@ -50,9 +51,10 @@ require (
 	github.com/prometheus/procfs v0.12.0 // indirect
 	github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect
 	github.com/zeebo/xxh3 v1.0.2 // indirect
-	go.uber.org/dig v1.19.0 // indirect
-	go.uber.org/multierr v1.10.0 // indirect
-	go.uber.org/zap v1.26.0 // indirect
+	go.uber.org/atomic v1.9.0 // indirect
+	go.uber.org/dig v1.17.0 // indirect
+	go.uber.org/multierr v1.9.0 // indirect
+	go.uber.org/zap v1.23.0 // indirect
 	golang.org/x/mod v0.17.0 // indirect
 	golang.org/x/sync v0.14.0 // indirect
 	golang.org/x/sys v0.47.0 // indirect

go.sum

@@ -1,5 +1,7 @@
 github.com/99designs/basicauth-go v0.0.0-20230316000542-bf6f9cbbf0f8 h1:nMpu1t4amK3vJWBibQ5X/Nv0aXL+b69TQf2uK5PH7Go=
 github.com/99designs/basicauth-go v0.0.0-20230316000542-bf6f9cbbf0f8/go.mod h1:3cARGAK9CfW3HoxCy1a0G4TKrdiKke8ftOMEOHyySYs=
+github.com/benbjohnson/clock v1.3.0 h1:ip6w0uFQkncKQ979AypyG0ER7mqUSBdKLOgAle/AT8A=
+github.com/benbjohnson/clock v1.3.0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZxNJlLklBHA=
 github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=
 github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw=
 github.com/cespare/xxhash/v2 v2.2.0 h1:DC2CZ1Ep5Y4k3ZQ899DldepgrayRUGE6BBZ/cd9Cj44=
@@ -10,6 +12,9 @@ github.com/chromedp/chromedp v0.16.0 h1:rOO4deOm4CbZgBCa8mD9g2rDyIoNs0BkgvNrlbp5
 github.com/chromedp/chromedp v0.16.0/go.mod h1:rbuGKFT1vMcFcFqKfPIO1GpX/N+2s8onm2qMxZLbU5U=
 github.com/chromedp/sysutil v1.1.0 h1:PUFNv5EcprjqXZD9nJb9b/c9ibAbxiYo4exNWZyipwM=
 github.com/chromedp/sysutil v1.1.0/go.mod h1:WiThHUdltqCNKGc4gaU50XgYjwjYIhKWoHGPTUfWTJ8=
+github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
+github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
+github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM=
 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY=
@@ -78,6 +83,7 @@ github.com/pingcap/errors v0.11.4 h1:lFuQV/oaUMGcD2tqt+01ROSmJs75VG1ToEOkZIZ4nE4
 github.com/pingcap/errors v0.11.4/go.mod h1:Oi8TUi2kEtXXLMJk9l1cGmz20kV3TaQ0usTwv5KuLY8=
 github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
 github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
+github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U=
 github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/prometheus/client_golang v1.18.0 h1:HzFfmkOzH5Q8L8G+kSJKUx5dtG87sewO+FoDDqP5Tbk=
@@ -94,24 +100,28 @@ github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjR
 github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog=
 github.com/slok/go-http-metrics v0.11.0 h1:ABJUpekCZSkQT1wQrFvS4kGbhea/w6ndFJaWJeh3zL0=
 github.com/slok/go-http-metrics v0.11.0/go.mod h1:ZGKeYG1ET6TEJpQx18BqAJAvxw9jBAZXCHU7bWQqqAc=
+github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY=
 github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA=
+github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
 github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U=
 github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U=
 github.com/zeebo/assert v1.3.0 h1:g7C04CbJuIDKNPFHmsk4hwZDO5O+kntRxzaUoNXj+IQ=
 github.com/zeebo/assert v1.3.0/go.mod h1:Pq9JiuJQpG8JLJdtkwrJESF0Foym2/D9XMU5ciN/wJ0=
 github.com/zeebo/xxh3 v1.0.2 h1:xZmwmqxHZA8AI603jOQ0tMqmBr9lPeFwGg6d+xy9DC0=
 github.com/zeebo/xxh3 v1.0.2/go.mod h1:5NWz9Sef7zIDm2JHfFlcQvNekmcEl9ekUZQQKCYaDcA=
-go.uber.org/dig v1.19.0 h1:BACLhebsYdpQ7IROQ1AGPjrXcP5dF80U3gKoFzbaq/4=
-go.uber.org/dig v1.19.0/go.mod h1:Us0rSJiThwCv2GteUN0Q7OKvU7n5J4dxZ9JKUXozFdE=
-go.uber.org/fx v1.24.0 h1:wE8mruvpg2kiiL1Vqd0CC+tr0/24XIB10Iwp2lLWzkg=
-go.uber.org/fx v1.24.0/go.mod h1:AmDeGyS+ZARGKM4tlH4FY2Jr63VjbEDJHtqXTGP5hbo=
-go.uber.org/goleak v1.2.0 h1:xqgm/S+aQvhWFTtR0XK3Jvg7z8kGV8P4X14IzwN3Eqk=
-go.uber.org/goleak v1.2.0/go.mod h1:XJYK+MuIchqpmGmUSAzotztawfKvYLUIgg7guXrwVUo=
-go.uber.org/multierr v1.10.0 h1:S0h4aNzvfcFsC3dRF1jLoaov7oRaKqRGC/pUEJ2yvPQ=
-go.uber.org/multierr v1.10.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y=
-go.uber.org/zap v1.26.0 h1:sI7k6L95XOKS281NhVKOFCUNIvv9e0w4BF8N3u+tCRo=
-go.uber.org/zap v1.26.0/go.mod h1:dtElttAiwGvoJ/vj4IwHBS/gXsEu/pZ50mUIRWuG0so=
+go.uber.org/atomic v1.9.0 h1:ECmE8Bn/WFTYwEW/bpKD3M8VtR/zQVbavAoalC1PYyE=
+go.uber.org/atomic v1.9.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
+go.uber.org/dig v1.17.0 h1:5Chju+tUvcC+N7N6EV08BJz41UZuO3BmHcN4A287ZLI=
+go.uber.org/dig v1.17.0/go.mod h1:rTxpf7l5I0eBTlE6/9RL+lDybC7WFwY2QH55ZSjy1mU=
+go.uber.org/fx v1.20.1 h1:zVwVQGS8zYvhh9Xxcu4w1M6ESyeMzebzj2NbSayZ4Mk=
+go.uber.org/fx v1.20.1/go.mod h1:iSYNbHf2y55acNCwCXKx7LbWb5WG1Bnue5RDXz1OREg=
+go.uber.org/goleak v1.1.11 h1:wy28qYRKZgnJTxGxvye5/wgWr1EKjmUDGYox5mGlRlI=
+go.uber.org/goleak v1.1.11/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ=
+go.uber.org/multierr v1.9.0 h1:7fIwc/ZtS0q++VgcfqFDxSBZVv/Xo49/SYnDFupUwlI=
+go.uber.org/multierr v1.9.0/go.mod h1:X2jQV1h+kxSjClGpnseKVIxpmcjrj7MNnI0bnlfKTVQ=
+go.uber.org/zap v1.23.0 h1:OjGQ5KQDEUawVHxNwQgPpiypGHOxo2mNZsOqTak4fFY=
+go.uber.org/zap v1.23.0/go.mod h1:D+nX8jyLsMHMYrln8A0rJjFt/T/9/bGgIhAqxv5URuY=
 golang.org/x/crypto v0.38.0 h1:jt+WWG8IZlBnVbomuhg2Mdq0+BBQaHbtqHEFEigjUV8=
 golang.org/x/crypto v0.38.0/go.mod h1:MvrbAqul58NNYPKnOra203SB9vpuZW0e+RRZV+Ggqjw=
 golang.org/x/mod v0.17.0 h1:zY54UmvipHiNd+pm+m0x9KhZ9hl1/7QNMyxXbc6ICqA=

internal/config/config.go

@@ -80,7 +80,8 @@ const (
 	// process over a Docker network or a private LAN connects from.
 	defaultTrustedProxies = "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16"
 
-	// maxPort is the highest valid TCP port number.
+	// maxPort is the highest valid TCP port number. The lower
+	// bound (at least 1) is enforced by envPositiveInt.
 	maxPort = 65535
 
 	// mappedV4Offset is the number of leading bits an IPv4-mapped
@@ -104,7 +105,7 @@ var ErrInvalidEnvironment = errors.New("invalid environment")
 var ErrNonPositiveValue = errors.New("value must be positive")
 
 // ErrInvalidPort is returned when an environment variable holding a
-// TCP port number is set to a number outside 1 to 65535.
+// TCP port number is set above the valid port range.
 var ErrInvalidPort = errors.New("invalid port")
 
 // ErrInvalidCIDR is returned when an environment variable holding a
@@ -362,27 +363,17 @@ func envPositiveInt(
 // envPort returns the value of the named environment variable parsed
 // as a TCP port number. Returns defaultValue if not set. A set value
 // that is unparseable, below 1, or above maxPort is a hard error
-// naming the key and the bad value; every out-of-range value wraps
-// ErrInvalidPort, including one too large or too small for an int.
+// naming the key and the bad value.
 func envPort(key string, defaultValue int) (int, error) {
-	v := os.Getenv(key)
-	if v == "" {
-		return defaultValue, nil
+	port, err := envPositiveInt(key, defaultValue)
+	if err != nil {
+		return 0, err
 	}
 
-	// strconv.ErrRange means a number too large or too small for an
-	// int, which is outside the port range as well.
-	port, err := strconv.Atoi(v)
-	if err != nil && !errors.Is(err, strconv.ErrRange) {
+	if port > maxPort {
 		return 0, fmt.Errorf(
-			"invalid integer for %s: %q: %w", key, v, err,
-		)
-	}
-
-	if err != nil || port < 1 || port > maxPort {
-		return 0, fmt.Errorf(
-			"%w: %s must be from 1 to %d, got %q",
-			ErrInvalidPort, key, maxPort, v,
+			"%w: %s must be at most %d, got %d",
+			ErrInvalidPort, key, maxPort, port,
 		)
 	}
 

internal/config/config_test.go

@@ -3,6 +3,7 @@ package config_test
 import (
 	"bytes"
 	"log/slog"
+	"os"
 	"testing"
 	"time"
 
@@ -70,12 +71,14 @@ func TestEnvironmentConfig(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
-
 			if tt.envValue != "" {
 				t.Setenv(
 					"WEBHOOKER_ENVIRONMENT", tt.envValue,
 				)
+			} else {
+				require.NoError(t, os.Unsetenv(
+					"WEBHOOKER_ENVIRONMENT",
+				))
 			}
 
 			for k, v := range tt.envVars {
@@ -196,11 +199,14 @@ func TestRetentionSweepInterval(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("RETENTION_SWEEP_INTERVAL", tt.value)
+			} else {
+				require.NoError(t, os.Unsetenv(
+					"RETENTION_SWEEP_INTERVAL",
+				))
 			}
 
 			if tt.expectError {
@@ -335,11 +341,14 @@ func TestSessionIdleTimeout(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("SESSION_IDLE_TIMEOUT", tt.value)
+			} else {
+				require.NoError(t, os.Unsetenv(
+					"SESSION_IDLE_TIMEOUT",
+				))
 			}
 
 			if tt.expectError {
@@ -388,12 +397,16 @@ func TestDefaultDataDir(t *testing.T) {
 		t.Run("env="+name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
-
 			if env != "" {
 				t.Setenv("WEBHOOKER_ENVIRONMENT", env)
+			} else {
+				require.NoError(t, os.Unsetenv(
+					"WEBHOOKER_ENVIRONMENT",
+				))
 			}
 
+			require.NoError(t, os.Unsetenv("DATA_DIR"))
+
 			var cfg *config.Config
 
 			app := fxtest.New(
@@ -433,9 +446,9 @@ func TestDataDirHelper(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
-
-			if set != "" {
+			if set == "" {
+				require.NoError(t, os.Unsetenv("DATA_DIR"))
+			} else {
 				t.Setenv("DATA_DIR", set)
 			}
 
@@ -498,11 +511,14 @@ func TestReceiverRateLimit(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("RECEIVER_RATE_LIMIT", tt.value)
+			} else {
+				require.NoError(t, os.Unsetenv(
+					"RECEIVER_RATE_LIMIT",
+				))
 			}
 
 			if tt.expectError {
@@ -614,11 +630,12 @@ func TestTrustedProxies(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("TRUSTED_PROXIES", tt.value)
+			} else {
+				require.NoError(t, os.Unsetenv("TRUSTED_PROXIES"))
 			}
 
 			if tt.expectError {
@@ -725,11 +742,14 @@ func TestAllowedEgressCIDRs(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("ALLOWED_EGRESS_CIDRS", tt.value)
+			} else {
+				require.NoError(
+					t, os.Unsetenv("ALLOWED_EGRESS_CIDRS"),
+				)
 			}
 
 			if tt.expectError {
@@ -797,10 +817,13 @@ func TestEgressAllowlistWarning(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", config.EnvironmentDev)
 
-			if tt.allowed != "" {
+			if tt.allowed == "" {
+				require.NoError(
+					t, os.Unsetenv("ALLOWED_EGRESS_CIDRS"),
+				)
+			} else {
 				t.Setenv("ALLOWED_EGRESS_CIDRS", tt.allowed)
 			}
 
@@ -933,14 +956,20 @@ func TestMetricsAuthConfig(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
-
 			if tt.username.set {
 				t.Setenv("METRICS_USERNAME", tt.username.value)
+			} else {
+				require.NoError(
+					t, os.Unsetenv("METRICS_USERNAME"),
+				)
 			}
 
 			if tt.password.set {
 				t.Setenv("METRICS_PASSWORD", tt.password.value)
+			} else {
+				require.NoError(
+					t, os.Unsetenv("METRICS_PASSWORD"),
+				)
 			}
 
 			if tt.expectError {

internal/config/dotenv_test.go

@@ -22,6 +22,17 @@ const malformedDotEnv = "PORT 19615\n" +
 	"this is not = valid ! syntax\n" +
 	"\"unclosed\n"
 
+// unsetDotEnvKey makes dotEnvKey genuinely absent for the duration of
+// the test and restores it afterwards. t.Setenv registers the restore;
+// the Unsetenv that follows is what the test actually needs, because a
+// variable set to the empty string is still present in os.Environ and
+// godotenv would refuse to overwrite it.
+func unsetDotEnvKey(t *testing.T) {
+	t.Helper()
+	t.Setenv(dotEnvKey, "placeholder")
+	require.NoError(t, os.Unsetenv(dotEnvKey))
+}
+
 // writeDotEnv writes contents to a .env file in a fresh temporary
 // directory and returns its path.
 func writeDotEnv(t *testing.T, contents string) string {
@@ -39,9 +50,9 @@ func writeDotEnv(t *testing.T, contents string) string {
 // normally rather than be refused for a file it was never meant to
 // have.
 //
-//nolint:paralleltest // ClearEnvForTest uses t.Setenv.
+//nolint:paralleltest // unsetDotEnvKey uses t.Setenv.
 func TestLoadDotEnv_MissingFileIsFine(t *testing.T) {
-	config.ClearEnvForTest(t)
+	unsetDotEnvKey(t)
 
 	absent := filepath.Join(t.TempDir(), config.DotEnvPath)
 	require.NoError(t, config.LoadDotEnvFileForTest(absent))
@@ -54,9 +65,9 @@ func TestLoadDotEnv_MissingFileIsFine(t *testing.T) {
 // reaches the environment, which is the whole reason the file is read
 // at all.
 //
-//nolint:paralleltest // ClearEnvForTest uses t.Setenv.
+//nolint:paralleltest // unsetDotEnvKey uses t.Setenv.
 func TestLoadDotEnv_AppliesValues(t *testing.T) {
-	config.ClearEnvForTest(t)
+	unsetDotEnvKey(t)
 
 	path := writeDotEnv(t, "# a comment\n"+dotEnvKey+"=from-dot-env\n")
 
@@ -82,9 +93,9 @@ func TestLoadDotEnv_RealEnvironmentWins(t *testing.T) {
 // reverts to its default; the process used to start that way with no
 // log line naming the file at all.
 //
-//nolint:paralleltest // ClearEnvForTest uses t.Setenv.
+//nolint:paralleltest // unsetDotEnvKey uses t.Setenv.
 func TestLoadDotEnv_MalformedFileAborts(t *testing.T) {
-	config.ClearEnvForTest(t)
+	unsetDotEnvKey(t)
 
 	path := writeDotEnv(
 		t, malformedDotEnv+dotEnvKey+"=from-dot-env\n",
@@ -132,7 +143,7 @@ func TestLoadDotEnv_UnreadableFileAborts(t *testing.T) {
 //
 //nolint:paralleltest // t.Chdir moves the whole process.
 func TestLoadDotEnv_ReadsTheWorkingDirectory(t *testing.T) {
-	config.ClearEnvForTest(t)
+	unsetDotEnvKey(t)
 
 	dir := t.TempDir()
 	require.NoError(t, os.WriteFile(

internal/config/env_test.go

@@ -1,6 +1,7 @@
 package config_test
 
 import (
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -120,10 +121,10 @@ func TestEnvBool(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
-
 			if tt.set {
 				t.Setenv(testEnvKey, tt.value)
+			} else {
+				require.NoError(t, os.Unsetenv(testEnvKey))
 			}
 
 			got, err := config.EnvBoolForTest(
@@ -144,62 +145,17 @@ func TestEnvBool(t *testing.T) {
 	}
 }
 
-// envIntCase is one row of the envPositiveInt and envPort tables.
-type envIntCase struct {
-	name        string
-	set         bool
-	value       string
-	expectError bool
-	errIs       error
-	expected    int
-}
-
-// runEnvIntCases runs each row through parse, which is
-// envPositiveInt or envPort, with testEnvKey set to the row's value
-// or left unset.
-func runEnvIntCases(
-	t *testing.T,
-	parse func(key string, defaultValue int) (int, error),
-	defaultValue int,
-	tests []envIntCase,
-) {
-	t.Helper()
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			// Cannot use t.Parallel() here because t.Setenv
-			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
-
-			if tt.set {
-				t.Setenv(testEnvKey, tt.value)
-			}
-
-			got, err := parse(testEnvKey, defaultValue)
-
-			if tt.expectError {
-				require.Error(t, err)
-				assert.Contains(t, err.Error(), testEnvKey)
-				assert.Contains(t, err.Error(), tt.value)
-
-				if tt.errIs != nil {
-					require.ErrorIs(t, err, tt.errIs)
-				}
-
-				return
-			}
-
-			require.NoError(t, err)
-			assert.Equal(t, tt.expected, got)
-		})
-	}
-}
-
-//nolint:paralleltest // runEnvIntCases uses t.Setenv.
 func TestEnvPositiveInt(t *testing.T) {
 	const defaultValue = 7
 
-	runEnvIntCases(t, config.EnvPositiveIntForTest, defaultValue, []envIntCase{
+	tests := []struct {
+		name        string
+		set         bool
+		value       string
+		expectError bool
+		errIs       error
+		expected    int
+	}{
 		{
 			name:     "unset returns the default integer",
 			expected: defaultValue,
@@ -236,14 +192,51 @@ func TestEnvPositiveInt(t *testing.T) {
 			expectError: true,
 			errIs:       config.ErrNonPositiveValue,
 		},
-	})
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Cannot use t.Parallel() here because t.Setenv
+			// is incompatible with parallel subtests.
+			if tt.set {
+				t.Setenv(testEnvKey, tt.value)
+			} else {
+				require.NoError(t, os.Unsetenv(testEnvKey))
+			}
+
+			got, err := config.EnvPositiveIntForTest(
+				testEnvKey, defaultValue,
+			)
+
+			if tt.expectError {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), testEnvKey)
+				assert.Contains(t, err.Error(), tt.value)
+
+				if tt.errIs != nil {
+					require.ErrorIs(t, err, tt.errIs)
+				}
+
+				return
+			}
+
+			require.NoError(t, err)
+			assert.Equal(t, tt.expected, got)
+		})
+	}
 }
 
-//nolint:paralleltest // runEnvIntCases uses t.Setenv.
 func TestEnvPort(t *testing.T) {
 	const defaultValue = 8080
 
-	runEnvIntCases(t, config.EnvPortForTest, defaultValue, []envIntCase{
+	tests := []struct {
+		name        string
+		set         bool
+		value       string
+		expectError bool
+		errIs       error
+		expected    int
+	}{
 		{
 			name:     "unset returns the default port",
 			expected: defaultValue,
@@ -271,14 +264,7 @@ func TestEnvPort(t *testing.T) {
 			set:         true,
 			value:       "0",
 			expectError: true,
-			errIs:       config.ErrInvalidPort,
-		},
-		{
-			name:        "negative is rejected",
-			set:         true,
-			value:       "-1",
-			expectError: true,
-			errIs:       config.ErrInvalidPort,
+			errIs:       config.ErrNonPositiveValue,
 		},
 		{
 			name:        "above the port range is rejected",
@@ -287,14 +273,37 @@ func TestEnvPort(t *testing.T) {
 			expectError: true,
 			errIs:       config.ErrInvalidPort,
 		},
-		{
-			name:        "too large for an int is rejected",
-			set:         true,
-			value:       "99999999999999999999",
-			expectError: true,
-			errIs:       config.ErrInvalidPort,
-		},
-	})
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Cannot use t.Parallel() here because t.Setenv
+			// is incompatible with parallel subtests.
+			if tt.set {
+				t.Setenv(testEnvKey, tt.value)
+			} else {
+				require.NoError(t, os.Unsetenv(testEnvKey))
+			}
+
+			got, err := config.EnvPortForTest(
+				testEnvKey, defaultValue,
+			)
+
+			if tt.expectError {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), testEnvKey)
+
+				if tt.errIs != nil {
+					require.ErrorIs(t, err, tt.errIs)
+				}
+
+				return
+			}
+
+			require.NoError(t, err)
+			assert.Equal(t, tt.expected, got)
+		})
+	}
 }
 
 // TestEnvBindAddress covers BIND_ADDRESS parsing.
@@ -310,10 +319,10 @@ func TestEnvBindAddress(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
-
 			if tt.set {
 				t.Setenv(testEnvKey, tt.value)
+			} else {
+				require.NoError(t, os.Unsetenv(testEnvKey))
 			}
 
 			got, err := config.EnvBindAddressForTest(
@@ -476,7 +485,6 @@ func TestNewRejectsBadEnvValues(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 			t.Setenv(tt.key, tt.value)
 
@@ -638,9 +646,14 @@ func sentryEnvValueCases() []badEnvValueCase {
 // break the legitimate unset case: absent variables still get their
 // documented defaults.
 func TestNewUsesDefaultsWhenUnset(t *testing.T) {
-	config.ClearEnvForTest(t)
 	t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
+	for _, key := range []string{
+		envKeyPort, envKeyDebug, envKeyBindAddress, envKeySentryDSN,
+	} {
+		require.NoError(t, os.Unsetenv(key))
+	}
+
 	cfg, err := buildConfig(t)
 	require.NoError(t, err)
 	require.NotNil(t, cfg)

internal/config/sentry_test.go

@@ -1,6 +1,7 @@
 package config_test
 
 import (
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -100,10 +101,10 @@ func TestEnvSentryDSN(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
-
 			if tt.set {
 				t.Setenv(envKeySentryDSN, tt.value)
+			} else {
+				require.NoError(t, os.Unsetenv(envKeySentryDSN))
 			}
 
 			got, err := config.EnvSentryDSNForTest(envKeySentryDSN)

internal/config/testing.go

@@ -1,50 +0,0 @@
-package config
-
-import (
-	"os"
-	"strings"
-	"testing"
-)
-
-// ClearEnvForTest unsets every variable in the process environment
-// for the rest of the test, so a test sees only the variables it sets
-// itself, not whatever the developer's shell exports. When the test
-// ends it leaves the environment exactly as it found it: each variable
-// it unset is put back, and any variable added since is removed.
-func ClearEnvForTest(t *testing.T) {
-	t.Helper()
-
-	present := make(map[string]bool)
-
-	for _, entry := range os.Environ() {
-		key, _, _ := strings.Cut(entry, "=")
-		present[key] = true
-
-		// t.Setenv registers the restore; the Unsetenv after it is
-		// what makes the key absent, since a key set to the empty
-		// string is still present, and godotenv will not overwrite a
-		// present key.
-		t.Setenv(key, "")
-
-		err := os.Unsetenv(key)
-		if err != nil {
-			t.Fatalf("unsetting %s: %v", key, err)
-		}
-	}
-
-	// A variable the test adds other than through t.Setenv, as loading
-	// a .env file does, has no restore of its own.
-	t.Cleanup(func() {
-		for _, entry := range os.Environ() {
-			key, _, _ := strings.Cut(entry, "=")
-			if present[key] {
-				continue
-			}
-
-			err := os.Unsetenv(key)
-			if err != nil {
-				t.Errorf("unsetting %s: %v", key, err)
-			}
-		}
-	})
-}

internal/config/testing_test.go

@@ -1,36 +0,0 @@
-package config_test
-
-import (
-	"os"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/config"
-)
-
-// TestClearEnvForTest_RemovesAddedVariables pins that a variable set
-// after the clear other than through t.Setenv, as a test's .env file
-// sets one, is gone once the test ends, so it cannot reach the tests
-// that run after it.
-//
-//nolint:paralleltest // ClearEnvForTest uses t.Setenv.
-func TestClearEnvForTest_RemovesAddedVariables(t *testing.T) {
-	// The outer clear keeps a value of the key exported in the shell
-	// from making it a variable the inner clear has to put back.
-	config.ClearEnvForTest(t)
-
-	t.Run("loads a .env file after the clear", func(t *testing.T) {
-		config.ClearEnvForTest(t)
-
-		path := writeDotEnv(t, dotEnvKey+"=from-dot-env\n")
-		require.NoError(t, config.LoadDotEnvFileForTest(path))
-		require.Equal(t, "from-dot-env", os.Getenv(dotEnvKey))
-	})
-
-	_, present := os.LookupEnv(dotEnvKey)
-	assert.False(
-		t, present,
-		"a variable set after the clear must not outlive the test",
-	)
-}

internal/delivery/engine_integration_test.go

@@ -355,14 +355,9 @@ func TestProcessRetryTask_SuccessfulRetry(t *testing.T) {
 
 	s := newISetup(t)
 
-	var receivedBody string
-
 	ts := httptest.NewServer(
 		http.HandlerFunc(
-			func(w http.ResponseWriter, r *http.Request) {
-				body, _ := io.ReadAll(r.Body)
-				receivedBody = string(body)
-
+			func(w http.ResponseWriter, _ *http.Request) {
 				w.WriteHeader(http.StatusOK)
 			},
 		),
@@ -402,8 +397,6 @@ func TestProcessRetryTask_SuccessfulRetry(t *testing.T) {
 		context.TODO(), &task,
 	)
 
-	assert.Equal(t, event.Body, receivedBody)
-
 	iAssertStatus(t, s.WebhookDB, d.ID,
 		database.DeliveryStatusDelivered,
 	)
@@ -450,14 +443,9 @@ func TestProcessRetryTask_LargeBody_FetchFromDB(
 
 	s := newISetup(t)
 
-	var receivedBody string
-
 	ts := httptest.NewServer(
 		http.HandlerFunc(
-			func(w http.ResponseWriter, r *http.Request) {
-				body, _ := io.ReadAll(r.Body)
-				receivedBody = string(body)
-
+			func(w http.ResponseWriter, _ *http.Request) {
 				w.WriteHeader(http.StatusOK)
 			},
 		),
@@ -494,8 +482,6 @@ func TestProcessRetryTask_LargeBody_FetchFromDB(
 		context.TODO(), &task,
 	)
 
-	assert.Equal(t, largeBody, receivedBody)
-
 	iAssertStatus(t, s.WebhookDB, d.ID,
 		database.DeliveryStatusDelivered,
 	)

internal/delivery/target_database_export_test.go

@@ -1,7 +1,6 @@
 package delivery_test
 
 import (
-	"bufio"
 	"bytes"
 	"compress/gzip"
 	"crypto/rand"
@@ -12,6 +11,7 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
+	"runtime/debug"
 	"testing"
 	"time"
 
@@ -302,8 +302,7 @@ func TestArchiveExport_SurvivesRename(t *testing.T) {
 }
 
 // heapPeak is an io.Writer that discards what it is given and records
-// the largest heap it saw at a write. It collects garbage before each
-// reading, so the heap it reads is what is still held.
+// the largest heap it saw at a write.
 type heapPeak struct {
 	max uint64
 }
@@ -311,7 +310,6 @@ type heapPeak struct {
 func (p *heapPeak) Write(b []byte) (int, error) {
 	var m runtime.MemStats
 
-	runtime.GC()
 	runtime.ReadMemStats(&m)
 	p.max = max(p.max, m.HeapAlloc)
 
@@ -319,19 +317,17 @@ func (p *heapPeak) Write(b []byte) (int, error) {
 }
 
 // TestArchiveExport_Streams proves an export holds neither the archive
-// nor its output in memory whole: exporting a 3 MiB archive grows the
+// nor its output in memory whole: exporting a 16 MiB archive grows the
 // heap by less than half of that. The bodies are random bytes in
 // base64, which gzip shrinks by only a quarter, so an export that read
 // every row before writing, or built the JSON or the gzipped file
-// before writing it, would hold at least 2.25 MiB at a write. A
-// streaming export holds about 1 MiB, most of it gzip's compressor,
-// which is why the archive is not smaller.
+// before writing it, would hold at least 12 MiB at a write.
 //
 //nolint:paralleltest // It measures the heap, which tests share.
 func TestArchiveExport_Streams(t *testing.T) {
 	const (
-		rows     = 96
-		bodySize = 32 << 10
+		rows     = 64
+		bodySize = 256 << 10
 		limit    = rows * bodySize / 2
 	)
 
@@ -356,11 +352,9 @@ func TestArchiveExport_Streams(t *testing.T) {
 
 	defer func() { require.NoError(t, export.Close()) }()
 
-	// Through a buffer, the heap is read once per 256 KiB of output
-	// rather than at each of gzip's small writes, which takes far
-	// longer.
-	peak := &heapPeak{}
-	buffered := bufio.NewWriterSize(peak, 256<<10)
+	// A low GC target collects garbage soon after it is made, so the
+	// heap at each write is close to what the export is holding.
+	defer debug.SetGCPercent(debug.SetGCPercent(10))
 
 	runtime.GC()
 
@@ -368,8 +362,9 @@ func TestArchiveExport_Streams(t *testing.T) {
 
 	runtime.ReadMemStats(&start)
 
-	require.NoError(t, writeExportTo(t, export, buffered))
-	require.NoError(t, buffered.Flush())
+	peak := &heapPeak{}
+
+	require.NoError(t, writeExportTo(t, export, peak))
 	assert.Less(t, peak.max, start.HeapAlloc+limit,
 		"heap at the start %d, at its peak %d", start.HeapAlloc, peak.max,
 	)

internal/gormlog/firstboot_test.go

@@ -117,8 +117,8 @@ func readFirstBootSecrets(
 }
 
 // bootAtDebug starts and stops the real application graph against
-// dataDir with DEBUG=true and nothing else set, and returns everything
-// it wrote to standard output.
+// dataDir with DEBUG=true, and returns everything it wrote to standard
+// output.
 //
 // config.New reads DEBUG from the environment exactly as the binary
 // does, internal/logger builds the handler it builds in production,
@@ -128,7 +128,6 @@ func readFirstBootSecrets(
 func bootAtDebug(t *testing.T, dataDir string) string {
 	t.Helper()
 
-	config.ClearEnvForTest(t)
 	t.Setenv("DEBUG", "true")
 	t.Setenv("DATA_DIR", dataDir)
 

internal/handlers/settings.go

@@ -77,8 +77,7 @@ func settingRows(cfg *config.Config) []settingRow {
 		{
 			"RETENTION_SWEEP_INTERVAL",
 			"How often the retention reaper and archive sweeper run " +
-				"(Go duration, must be positive). A value that does " +
-				"not parse, or is zero or negative, fails startup",
+				"(Go duration, must be positive)",
 			cfg.RetentionSweepInterval.String(),
 		},
 		{

internal/handlers/target_download_test.go

@@ -1,15 +1,11 @@
 package handlers_test
 
 import (
-	"bytes"
 	"compress/gzip"
-	"context"
 	"crypto/rand"
 	"encoding/json"
 	"errors"
 	"io"
-	"log/slog"
-	"net"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -17,12 +13,11 @@ import (
 	"testing"
 	"time"
 
+	chimw "github.com/go-chi/chi/middleware"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/delivery"
-	"sneak.berlin/go/webhooker/internal/middleware"
 )
 
 // errClientGone is the write failure of a client that has gone away.
@@ -225,59 +220,20 @@ func seedArchive(t *testing.T, path string, rows, bodySize int) {
 	}
 }
 
-// limitedServer serves the target routes as the server does, behind the
-// access log, whose lines it returns, and the request limit, here
-// limit, which is also its write timeout. Each connection's send buffer
-// is a few KiB, so a larger response is still being written while its
-// client is not reading.
-func limitedServer(
-	t *testing.T, env *sourceTestEnv, limit time.Duration,
-) (*httptest.Server, *bytes.Buffer) {
-	t.Helper()
-
-	const sendBuffer = 4 << 10
-
-	logBuf := new(bytes.Buffer)
-	mw := middleware.NewForTest(
-		slog.New(slog.NewJSONHandler(logBuf, nil)),
-		&config.Config{Environment: config.EnvironmentDev},
-		nil,
-	)
-
-	srv := httptest.NewUnstartedServer(
-		mw.Logging()(mw.Timeout(limit)(targetRouter(env))),
-	)
-	srv.Config.WriteTimeout = limit
-	srv.Config.ConnContext = func(
-		ctx context.Context, c net.Conn,
-	) context.Context {
-		tcp, ok := c.(*net.TCPConn)
-		if assert.True(t, ok) {
-			assert.NoError(t, tcp.SetWriteBuffer(sendBuffer))
-		}
-
-		return ctx
-	}
-	srv.Start()
-	t.Cleanup(srv.Close)
-
-	return srv, logBuf
-}
-
 // TestHandleTargetDownload_OutlastsTheRequestLimit proves a download
-// runs for as long as the client keeps reading, and is logged as the
-// 200 it was. Behind a request limit and a server write timeout of a
-// tenth of a second, the client stops reading once the response has
-// started, waits three times as long, and still gets the whole file.
-// The archive is larger than the connection holds, so the download is
-// still being written while the client waits.
+// runs for as long as the client keeps reading. Behind a request limit
+// and a server write timeout of a tenth of a second, the client stops
+// reading once the response has started, waits three times as long,
+// and still gets the whole file. The archive is larger than a
+// connection holds, so the download is still being written while the
+// client waits.
 func TestHandleTargetDownload_OutlastsTheRequestLimit(t *testing.T) {
 	t.Parallel()
 
 	const (
 		limit    = 100 * time.Millisecond
-		rows     = 8
-		bodySize = 64 << 10
+		rows     = 12
+		bodySize = 1 << 20
 	)
 
 	env := setupSourceTest(t)
@@ -287,7 +243,12 @@ func TestHandleTargetDownload_OutlastsTheRequestLimit(t *testing.T) {
 		t, delivery.ArchivePath(env.dbMgr, &wh, archive), rows, bodySize,
 	)
 
-	srv, accessLog := limitedServer(t, env, limit)
+	srv := httptest.NewUnstartedServer(
+		chimw.Timeout(limit)(targetRouter(env)),
+	)
+	srv.Config.WriteTimeout = limit
+	srv.Start()
+	t.Cleanup(srv.Close)
 
 	req, err := http.NewRequestWithContext(
 		t.Context(), http.MethodGet,
@@ -324,18 +285,6 @@ func TestHandleTargetDownload_OutlastsTheRequestLimit(t *testing.T) {
 	// finished.
 	_, err = io.ReadAll(zr)
 	require.NoError(t, err)
-
-	// Close waits for the handler, so the access log line is written.
-	srv.Close()
-
-	var access map[string]any
-
-	require.NoError(t, json.Unmarshal(accessLog.Bytes(), &access))
-	assert.EqualValues(t, http.StatusOK, access["status"])
-	assert.GreaterOrEqual(t,
-		access["latency_ms"], float64(limit.Milliseconds()),
-		"the download must outlast the request limit",
-	)
 }
 
 // brokenWriter is a response writer whose writes fail once the

internal/logfield/logfield_test.go

@@ -5,7 +5,6 @@ import (
 	"io"
 	"log/slog"
 	"strings"
-	"sync"
 	"testing"
 	"unicode/utf8"
 
@@ -19,11 +18,15 @@ import (
 // width.
 const budget = 64
 
-// batchRunes is how many consecutive code points the charge test logs
-// in one value from U+1000 up. Logging each of those on its own line
-// is too slow for the suite under the race detector; 4,096 at a time
-// is 271 batches, each logged on two lines, so 542 lines per handler.
-const batchRunes = 4096
+// sampleRunes is how many runes wide the values in the charge test
+// are. The handlers add a constant per field — a pair of quotes when
+// the value needs quoting — so the per-rune charge is only visible
+// once it is amortised over a run of them.
+const sampleRunes = 64
+
+// quotingSlack is that constant: the pair of quotes a handler adds to
+// a value that needs them and omits from one that does not.
+const quotingSlack = 2
 
 // newHandlers are the two handlers internal/logger can install. Time
 // is dropped so a line's width is a function of its value alone —
@@ -63,48 +66,46 @@ func renderedWidth(
 	return buf.Len()
 }
 
-// emittedBytes is what a handler writes for the runes of s alone, in a
-// value that starts with prefix: the width of a line carrying prefix
-// and then s twice, less that of a line carrying prefix and s once.
-// Both values start the same way and hold the same runes, so the text
-// handler quotes both or neither, and the quotes cancel along with the
-// prefix and everything else on the line.
-func emittedBytes(
-	newHandler func(io.Writer) slog.Handler,
-	prefix, s string,
-) int {
-	return renderedWidth(newHandler, prefix+s+s) -
-		renderedWidth(newHandler, prefix+s)
-}
+// chargeTestRunes is the set of code points the charge test measures:
+// every rune in the first two planes' worth of the BMP that the
+// handlers are most likely to treat specially, the separators that
+// only slog's JSON handler escapes, and a stratified sample across
+// the rest of Unicode so the astral charge is exercised on more than
+// one hand-picked rune.
+func chargeTestRunes() []rune {
+	const (
+		denseCeiling = 0x800
+		stride       = 1021
+		surrogateLo  = 0xD800
+		surrogateHi  = 0xDFFF
+	)
 
-// firstUndercharged returns the first rune in s that the handler
-// writes in more bytes than EncodedBytes charges for it, and how many
-// runes in s are undercharged that way. It measures one rune per line,
-// in a value of that rune alone and again after a space, which makes
-// the text handler quote the value. The charge test calls it on the
-// code points below U+1000, and from there up only on a batch that has
-// already failed, to name the code points rather than just their range.
-func firstUndercharged(
-	newHandler func(io.Writer) slog.Handler,
-	s string,
-) (rune, int) {
-	first, count := rune(-1), 0
+	var runes []rune
 
-	for _, r := range s {
-		charge := logfield.EncodedBytes(r)
-		if emittedBytes(newHandler, "", string(r)) <= charge &&
-			emittedBytes(newHandler, " ", string(r)) <= charge {
-			continue
+	keep := func(r rune) {
+		if r >= surrogateLo && r <= surrogateHi {
+			return
 		}
 
-		if count == 0 {
-			first = r
-		}
-
-		count++
+		runes = append(runes, r)
 	}
 
-	return first, count
+	for r := range rune(denseCeiling) {
+		keep(r)
+	}
+
+	for _, r := range []rune{
+		0x2028, 0x2029, 0x200B, 0x4E00, 0xE000, 0xFFFD,
+		0x1000C, 0x1F600, 0xE0001, 0x10FFFF,
+	} {
+		keep(r)
+	}
+
+	for r := rune(denseCeiling); r <= utf8.MaxRune; r += stride {
+		keep(r)
+	}
+
+	return runes
 }
 
 // TestEncodedBytes_ChargesAtLeastWhatTheHandlersEmit is the property
@@ -113,93 +114,33 @@ func firstUndercharged(
 // how a stated ceiling becomes false without any test noticing, so
 // the charge is measured against what the handlers actually write
 // rather than against the escaping rules as read.
-//
-// Every code point below U+1000 is checked on its own, for both
-// handlers. That range holds the quote, the backslash and the control
-// characters the handlers escape, next to code points each handler
-// writes in fewer bytes than their charge, which in a sum would cover
-// a neighbour charged too little. Each is measured in a value of it
-// alone and again in one the text handler quotes, because that handler
-// writes U+007F as one raw byte in a value it leaves bare but as \x7f,
-// four bytes, in one it quotes.
-//
-// From U+1000 up the text handler writes every code point in exactly
-// its charge, so the rest of Unicode is checked batchRunes at a time:
-// each batch's summed charge must cover what the handler writes for
-// the whole batch. The sums there can miss the JSON handler alone
-// writing one code point in more bytes than its charge, when it writes
-// others in the same batch in fewer.
 func TestEncodedBytes_ChargesAtLeastWhatTheHandlersEmit(t *testing.T) {
 	t.Parallel()
 
-	var below strings.Builder
-	for r := range rune(0x1000) {
-		below.WriteRune(r)
-	}
-
-	var batches []string
-
-	for lo := rune(0x1000); lo <= utf8.MaxRune; lo += batchRunes {
-		var batch strings.Builder
-
-		for r := lo; r < lo+batchRunes; r++ {
-			// Surrogate halves are not runes a string can carry.
-			if utf8.ValidRune(r) {
-				batch.WriteRune(r)
-			}
-		}
-
-		batches = append(batches, batch.String())
-	}
-
-	// What EncodedBytes charges for each batch. Under -race -cover this
-	// takes longer than logging the batches, so it is worked out once,
-	// by whichever handler finishes logging first, while the other is
-	// still logging.
-	charged := sync.OnceValue(func() []int {
-		costs := make([]int, len(batches))
-
-		for i, batch := range batches {
-			for _, r := range batch {
-				costs[i] += logfield.EncodedBytes(r)
-			}
-		}
-
-		return costs
-	})
-
 	for name, newHandler := range newHandlers() {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			if first, count := firstUndercharged(newHandler, below.String()); count > 0 {
-				t.Errorf(
-					"%d code points below U+1000 cost more than "+
-						"EncodedBytes charges, the first U+%04X",
-					count, first,
+			// 'a' is a printable ASCII rune, charged exactly one
+			// byte, so it is the zero point the other runes are
+			// measured against.
+			base := renderedWidth(
+				newHandler, strings.Repeat("a", sampleRunes),
+			)
+
+			for _, r := range chargeTestRunes() {
+				got := renderedWidth(
+					newHandler,
+					strings.Repeat(string(r), sampleRunes),
 				)
-			}
+				charged := sampleRunes *
+					(logfield.EncodedBytes(r) - 1)
 
-			emitted := make([]int, len(batches))
-			for i, batch := range batches {
-				emitted[i] = emittedBytes(newHandler, "", batch)
-			}
-
-			for i, cost := range charged() {
-				if emitted[i] <= cost {
-					continue
-				}
-
-				lo := rune(0x1000 + i*batchRunes)
-				first, count := firstUndercharged(
-					newHandler, batches[i],
-				)
-				t.Errorf(
-					"U+%04X to U+%04X emit %d bytes but are "+
-						"charged %d; %d of them cost more than "+
-						"EncodedBytes charges, the first U+%04X",
-					lo, lo+batchRunes-1, emitted[i], cost,
-					count, first,
+				require.LessOrEqual(
+					t, got-base, charged+quotingSlack,
+					"U+%04X costs more on the line than "+
+						"EncodedBytes charges for it",
+					r,
 				)
 			}
 		})

internal/logger/logger.go

@@ -9,7 +9,6 @@ import (
 	"time"
 
 	"go.uber.org/fx"
-	"go.uber.org/fx/fxevent"
 	"sneak.berlin/go/webhooker/internal/globals"
 )
 
@@ -107,40 +106,3 @@ func (l *Logger) Identify() {
 func (l *Logger) Writer() io.Writer {
 	return os.Stdout
 }
-
-// FxLogger writes fx's own events through a slog logger: how the
-// dependency graph was built at DEBUG, since it repeats on every
-// start; the start and stop hooks, the start itself and the signal
-// that stops the service at INFO; every failure at ERROR.
-//
-// The formatting is fx's own fxevent.SlogLogger. That logger takes a
-// single level for every event that is not a failure, so FxLogger
-// holds one at each level and picks between them.
-type FxLogger struct {
-	graph     *fxevent.SlogLogger
-	lifecycle *fxevent.SlogLogger
-}
-
-// NewFxLogger returns an FxLogger that writes through log.
-func NewFxLogger(log *slog.Logger) *FxLogger {
-	graph := &fxevent.SlogLogger{Logger: log}
-	graph.UseLogLevel(slog.LevelDebug)
-
-	lifecycle := &fxevent.SlogLogger{Logger: log}
-	lifecycle.UseLogLevel(slog.LevelInfo)
-
-	return &FxLogger{graph: graph, lifecycle: lifecycle}
-}
-
-// LogEvent implements fxevent.Logger.
-func (f *FxLogger) LogEvent(event fxevent.Event) {
-	switch event.(type) {
-	case *fxevent.Supplied, *fxevent.Provided, *fxevent.Replaced,
-		*fxevent.Decorated, *fxevent.BeforeRun, *fxevent.Run,
-		*fxevent.Invoking, *fxevent.Invoked,
-		*fxevent.LoggerInitialized:
-		f.graph.LogEvent(event)
-	default:
-		f.lifecycle.LogEvent(event)
-	}
-}

internal/logger/logger_test.go

@@ -1,23 +1,13 @@
 package logger_test
 
 import (
-	"bytes"
-	"encoding/json"
-	"errors"
-	"log/slog"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"go.uber.org/fx"
-	"go.uber.org/fx/fxevent"
 	"go.uber.org/fx/fxtest"
 	"sneak.berlin/go/webhooker/internal/globals"
 	"sneak.berlin/go/webhooker/internal/logger"
 )
 
-var errStopHook = errors.New("stop hook failed on purpose")
-
 func testGlobals() *globals.Globals {
 	return &globals.Globals{
 		Appname: "test-app",
@@ -67,47 +57,3 @@ func TestEnableDebugLogging(t *testing.T) {
 	// Test debug logging
 	l.Get().Debug("debug message", "test", true)
 }
-
-// TestFxLogger_Levels starts and stops an fx app that reports its own
-// events through NewFxLogger, as cmd/webhooker does, and reads back
-// what reached the handler: the graph at DEBUG, the start at INFO and
-// a failed stop hook at ERROR, each as a structured record.
-func TestFxLogger_Levels(t *testing.T) {
-	t.Parallel()
-
-	var out bytes.Buffer
-
-	log := slog.New(slog.NewJSONHandler(
-		&out, &slog.HandlerOptions{Level: slog.LevelDebug},
-	))
-
-	app := fx.New(
-		fx.WithLogger(func() fxevent.Logger {
-			return logger.NewFxLogger(log)
-		}),
-		fx.Invoke(func(lc fx.Lifecycle) {
-			lc.Append(fx.StopHook(func() error { return errStopHook }))
-		}),
-	)
-
-	require.NoError(t, app.Start(t.Context()))
-	require.ErrorIs(t, app.Stop(t.Context()), errStopHook)
-
-	levels := map[string]string{}
-
-	decoder := json.NewDecoder(&out)
-	for decoder.More() {
-		var record struct {
-			Level string `json:"level"`
-			Msg   string `json:"msg"`
-		}
-
-		require.NoError(t, decoder.Decode(&record))
-
-		levels[record.Msg] = record.Level
-	}
-
-	assert.Equal(t, "DEBUG", levels["provided"])
-	assert.Equal(t, "INFO", levels["started"])
-	assert.Equal(t, "ERROR", levels["OnStop hook failed"])
-}

internal/middleware/middleware.go

@@ -600,10 +600,7 @@ func bodyLimitedMethod(method string) bool {
 }
 
 // MaxBodySize returns middleware that limits the size of
-// POST/PUT/PATCH request bodies to maxBytes. A request with any other
-// method passes through uncapped, deliberately: no route behind it
-// reads a body on GET, HEAD or DELETE. A handler that starts to needs
-// its method added to bodyLimitedMethod first. It must be registered
+// POST/PUT/PATCH request bodies to maxBytes. It must be registered
 // before any middleware that parses the body — notably CSRF, which
 // calls r.PostFormValue — so that form parsing happens under this
 // cap rather than net/http's 10 MB default.

internal/middleware/middleware_test.go

@@ -730,8 +730,10 @@ func TestNoCache_SetsHeaders(t *testing.T) {
 
 const testBodyLimit int64 = 64
 
-// maxBodySizeResult is what runMaxBodySize's sentinel handler saw,
-// together with the response.
+// maxBodySizeHandler wraps a sentinel handler in MaxBodySize with
+// testBodyLimit. The sentinel records whether it ran and how much of
+// the body it managed to read, so tests can distinguish "never
+// reached" from "reached but truncated".
 type maxBodySizeResult struct {
 	called   bool
 	read     int
@@ -739,10 +741,6 @@ type maxBodySizeResult struct {
 	response *httptest.ResponseRecorder
 }
 
-// runMaxBodySize wraps a sentinel handler in MaxBodySize with
-// testBodyLimit and serves req through it. The sentinel records
-// whether it ran and how much of the body it managed to read, so
-// tests can distinguish "never reached" from "reached but truncated".
 func runMaxBodySize(
 	t *testing.T,
 	req *http.Request,

internal/middleware/ratelimit.go

@@ -219,11 +219,7 @@ func (m *Middleware) clientKey(r *http.Request) string {
 		// path cannot silently collapse unrelated clients
 		// together. On a Unix-socket listener every peer
 		// carries the same RemoteAddr and so shares one bucket,
-		// which is the fail-closed direction. An empty RemoteAddr
-		// is a different case, which net/http never produces for
-		// a TCP listener and only a hand-built request carries,
-		// but it fails closed the same way: every such request
-		// shares the one bucket keyed on the empty string.
+		// which is the fail-closed direction.
 		return r.RemoteAddr
 	}
 

internal/middleware/ratelimit_test.go

@@ -1012,23 +1012,6 @@ func TestRateLimitKey_UnparseablePeerKeepsDistinctBuckets(
 	)
 }
 
-// TestRateLimitKey_EmptyPeerSharesOneBucket pins what the fallback
-// does with an empty RemoteAddr: it keys on the empty string, so every
-// such request shares one bucket. That is the fail-closed direction
-// and is kept on purpose; only a hand-built request carries an empty
-// RemoteAddr.
-func TestRateLimitKey_EmptyPeerSharesOneBucket(t *testing.T) {
-	t.Parallel()
-
-	m := rateLimitMiddleware(t, &config.Config{})
-
-	assert.Empty(
-		t, clientKeyFor(t, m, ""),
-		"every peer with an empty RemoteAddr must key on the "+
-			"empty string and so share one bucket",
-	)
-}
-
 // TestPostRateLimit_IPv6SharesBucketWithinSlash64 is the behavioural
 // half, and the regression test for the bypass itself: a client that
 // rotates source addresses inside its own routed /64 must stay in one

internal/middleware/timeout.go

@@ -1,71 +0,0 @@
-package middleware
-
-import (
-	"context"
-	"errors"
-	"net/http"
-	"time"
-)
-
-// Timeout returns middleware that gives each request limit to finish:
-// it cancels the request's context once limit has passed, and answers
-// 504 when the handler then returns without having started its
-// response.
-//
-// It replaces chi's middleware.Timeout, which writes that 504 even
-// after the handler has sent its own status. A download that outlasts
-// the limit has already sent its 200 and the whole file, so the late
-// 504 changes nothing for the client: the access log and the metrics
-// would record it in place of the 200, and net/http would complain of
-// a superfluous WriteHeader.
-func (s *Middleware) Timeout(
-	limit time.Duration,
-) func(http.Handler) http.Handler {
-	return func(next http.Handler) http.Handler {
-		return http.HandlerFunc(func(
-			w http.ResponseWriter,
-			r *http.Request,
-		) {
-			ctx, cancel := context.WithTimeout(r.Context(), limit)
-			defer cancel()
-
-			tw := &timeoutResponseWriter{ResponseWriter: w}
-
-			next.ServeHTTP(tw, r.WithContext(ctx))
-
-			if !tw.started &&
-				errors.Is(ctx.Err(), context.DeadlineExceeded) {
-				w.WriteHeader(http.StatusGatewayTimeout)
-			}
-		})
-	}
-}
-
-// timeoutResponseWriter records whether the handler has started its
-// response.
-type timeoutResponseWriter struct {
-	http.ResponseWriter
-
-	started bool
-}
-
-func (w *timeoutResponseWriter) WriteHeader(code int) {
-	w.started = true
-
-	w.ResponseWriter.WriteHeader(code)
-}
-
-func (w *timeoutResponseWriter) Write(b []byte) (int, error) {
-	// A Write without a WriteHeader starts the response too: net/http
-	// sends 200 in front of it.
-	w.started = true
-
-	//nolint:wrapcheck // Pass the writer's own error through unchanged.
-	return w.ResponseWriter.Write(b)
-}
-
-// Unwrap lets http.ResponseController reach the writer underneath, so
-// a handler can still set a write deadline through this wrapper.
-func (w *timeoutResponseWriter) Unwrap() http.ResponseWriter {
-	return w.ResponseWriter
-}

internal/middleware/timeout_test.go

@@ -1,56 +0,0 @@
-package middleware_test
-
-import (
-	"net/http"
-	"net/http/httptest"
-	"testing"
-	"time"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-// TestTimeout proves the request limit answers 504 to a handler that
-// outlasts it without starting its response, and leaves a response the
-// handler has started with the status it sent. Both are what the
-// access log records.
-func TestTimeout(t *testing.T) {
-	t.Parallel()
-
-	const limit = 10 * time.Millisecond
-
-	for _, tc := range []struct {
-		name string
-		sent int // the status the handler sends, or 0 for none
-		want int
-	}{
-		{name: "not started", sent: 0, want: http.StatusGatewayTimeout},
-		{name: "started", sent: http.StatusOK, want: http.StatusOK},
-	} {
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-
-			m, buf := capturingMiddleware(t)
-			handler := m.Logging()(m.Timeout(limit)(http.HandlerFunc(
-				func(w http.ResponseWriter, r *http.Request) {
-					if tc.sent != 0 {
-						w.WriteHeader(tc.sent)
-					}
-
-					<-r.Context().Done()
-				},
-			)))
-
-			w := httptest.NewRecorder()
-			handler.ServeHTTP(w, httptest.NewRequestWithContext(
-				t.Context(), http.MethodGet, "/", nil,
-			))
-
-			assert.Equal(t, tc.want, w.Code)
-
-			entries := accessLogEntries(t, buf)
-			require.Len(t, entries, 1)
-			assert.EqualValues(t, tc.want, entries[0]["status"])
-		})
-	}
-}

internal/server/error_page_test.go

@@ -191,7 +191,7 @@ func TestErrorPage_PanicOnAdminPage(t *testing.T) {
 
 	w := serve(
 		server.NewRouterWithPageProbeForTest(
-			t, env.log, env.cfg, env.mw, env.hnd,
+			env.log.Get(), env.cfg, env.mw, env.hnd,
 			true, panicProbeHandler,
 		),
 		server.PageProbePattern,
@@ -200,7 +200,7 @@ func TestErrorPage_PanicOnAdminPage(t *testing.T) {
 
 	w = serve(
 		server.NewRouterWithProbeForTest(
-			t, env.log, env.cfg, env.mw, env.hnd,
+			env.log.Get(), env.cfg, env.mw, env.hnd,
 			true, panicProbeHandler,
 		),
 		server.ProbePattern,

internal/server/export_test.go

@@ -1,16 +1,13 @@
 package server
 
 import (
+	"log/slog"
 	"net/http"
-	"testing"
 
 	"github.com/getsentry/sentry-go"
 	"github.com/go-chi/chi"
-	"github.com/stretchr/testify/require"
-	"go.uber.org/fx/fxtest"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/handlers"
-	"sneak.berlin/go/webhooker/internal/logger"
 	"sneak.berlin/go/webhooker/internal/middleware"
 )
 
@@ -37,45 +34,23 @@ func SentryClientOptionsForTest(
 	return sentryClientOptions(dsn, release)
 }
 
-// newServerForTest builds a Server through New, as the application
-// does, on a lifecycle that is never started: the hooks New adds to
-// it never run, so nothing listens.
-func newServerForTest(
-	t *testing.T,
-	log *logger.Logger,
-	cfg *config.Config,
-	mw *middleware.Middleware,
-	h *handlers.Handlers,
-) *Server {
-	t.Helper()
-
-	s, err := New(fxtest.NewLifecycle(t), ServerParams{
-		Logger:     log,
-		Config:     cfg,
-		Middleware: mw,
-		Handlers:   h,
-	})
-	require.NoError(t, err)
-
-	return s
-}
-
 // NewRouterForTest builds the real route tree via SetupRoutes with
-// the supplied middleware and handlers, on a Server from New whose
-// lifecycle is never started, so no HTTP listener runs. Tests use it
-// so that route-group middleware registration order is exercised
-// exactly as it ships, rather than against a hand-rebuilt chain that
-// could drift from routes.go.
+// the supplied middleware and handlers, bypassing the fx lifecycle
+// and the HTTP listener. Tests use it so that route-group middleware
+// registration order is exercised exactly as it ships, rather than
+// against a hand-rebuilt chain that could drift from routes.go.
 func NewRouterForTest(
-	t *testing.T,
-	log *logger.Logger,
+	log *slog.Logger,
 	cfg *config.Config,
 	mw *middleware.Middleware,
 	h *handlers.Handlers,
 ) http.Handler {
-	t.Helper()
-
-	s := newServerForTest(t, log, cfg, mw, h)
+	s := &Server{
+		log:    log,
+		mw:     mw,
+		h:      h,
+		params: ServerParams{Config: cfg},
+	}
 	s.SetupRoutes()
 
 	return s.router
@@ -108,17 +83,19 @@ const ProbePattern = "/probe"
 // option and the recoverer registered outside it is the thing a test
 // has to be able to pin.
 func NewRouterWithProbeForTest(
-	t *testing.T,
-	log *logger.Logger,
+	log *slog.Logger,
 	cfg *config.Config,
 	mw *middleware.Middleware,
 	h *handlers.Handlers,
 	sentryEnabled bool,
 	probe http.HandlerFunc,
 ) http.Handler {
-	t.Helper()
-
-	s := newServerForTest(t, log, cfg, mw, h)
+	s := &Server{
+		log:    log,
+		mw:     mw,
+		h:      h,
+		params: ServerParams{Config: cfg},
+	}
 	s.sentryEnabled.Store(sentryEnabled)
 	s.SetupRoutes()
 	s.router.Handle(ProbePattern, probe)
@@ -136,17 +113,19 @@ const PageProbePattern = "/pages/probe"
 // it, so the probe runs behind that group's own middleware exactly as
 // the group's real routes do.
 func NewRouterWithPageProbeForTest(
-	t *testing.T,
-	log *logger.Logger,
+	log *slog.Logger,
 	cfg *config.Config,
 	mw *middleware.Middleware,
 	h *handlers.Handlers,
 	sentryEnabled bool,
 	probe http.HandlerFunc,
 ) http.Handler {
-	t.Helper()
-
-	s := newServerForTest(t, log, cfg, mw, h)
+	s := &Server{
+		log:    log,
+		mw:     mw,
+		h:      h,
+		params: ServerParams{Config: cfg},
+	}
 	s.sentryEnabled.Store(sentryEnabled)
 	s.SetupRoutes()
 

internal/server/recoverer_test.go

@@ -199,7 +199,7 @@ func TestPanicProbeChild(t *testing.T) {
 	env := newTestEnv(t)
 
 	router := server.NewRouterWithProbeForTest(
-		t, env.log, env.cfg, env.mw, env.hnd,
+		env.log.Get(), env.cfg, env.mw, env.hnd,
 		false, panicProbeHandler,
 	)
 
@@ -253,7 +253,7 @@ func TestSentryStillSeesAPanic(t *testing.T) {
 	require.NoError(t, err)
 
 	router := server.NewRouterWithProbeForTest(
-		t, env.log, env.cfg, env.mw, env.hnd,
+		env.log.Get(), env.cfg, env.mw, env.hnd,
 		true, panicProbeHandler,
 	)
 

internal/server/response_controller_test.go

@@ -67,11 +67,11 @@ func TestResponseControllerThroughProductionRouter(t *testing.T) {
 
 			routers := map[string]http.Handler{
 				server.ProbePattern: server.NewRouterWithProbeForTest(
-					t, env.log, env.cfg, env.mw, env.hnd,
+					env.log.Get(), env.cfg, env.mw, env.hnd,
 					tc.sentryEnabled, probe,
 				),
 				server.PageProbePattern: server.NewRouterWithPageProbeForTest(
-					t, env.log, env.cfg, env.mw, env.hnd,
+					env.log.Get(), env.cfg, env.mw, env.hnd,
 					tc.sentryEnabled, probe,
 				),
 			}

internal/server/routes.go

@@ -73,7 +73,7 @@ func (s *Server) setupGlobalMiddleware() {
 	}
 
 	s.router.Use(s.mw.CORS())
-	s.router.Use(s.mw.Timeout(requestTimeout))
+	s.router.Use(middleware.Timeout(requestTimeout))
 
 	// Panic recovery, deliberately here rather than first. It has to
 	// run inside every middleware that observes the response, so the

internal/server/routes_test.go

@@ -136,7 +136,7 @@ func newTestEnvWithConfig(
 	t.Cleanup(app.RequireStop)
 
 	return &testEnv{
-		router: server.NewRouterForTest(t, log, cfg, mw, hnd),
+		router: server.NewRouterForTest(log.Get(), cfg, mw, hnd),
 		sess:   sess,
 		db:     db,
 		dbMgr:  dbMgr,
@@ -531,48 +531,6 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
 	}
 }
 
-// --- every page route group ---
-
-// TestPageRouteGroups_OversizeBody_RejectedBeforeCSRF pins the body
-// cap ahead of CSRF and RequireAuth in every page route group. The
-// requests carry no session and no CSRF token, so if either ran first
-// the answer would be a 403 or a redirect to the login page rather
-// than 413, and CSRF would issue its cookie (see
-// TestPagesLogin_UnderLimit_NoToken_CSRFRejects). /settings has no
-// POST route, but its group's middleware runs before the method is
-// matched, so a POST there still reaches CSRF's form parsing if the
-// cap moves after it. The user and webhook in the paths need not
-// exist: nothing after the cap runs.
-func TestPageRouteGroups_OversizeBody_RejectedBeforeCSRF(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	env := newTestEnv(t)
-
-	form := url.Values{}
-	form.Set("name", oversizeValue())
-
-	for _, path := range []string{
-		"/pages/login",
-		"/user/nobody/password",
-		"/settings/",
-		"/hooks/new",
-		"/hook/nonexistent/edit",
-	} {
-		w := env.post(path, form, nil)
-
-		assert.Equal(
-			t, http.StatusRequestEntityTooLarge, w.Code, path,
-		)
-		assert.False(
-			t, csrfCookieSet(w),
-			"CSRF middleware must not run for an oversized body to %s",
-			path,
-		)
-	}
-}
-
 // --- /pages group ---
 
 // TestPagesLogin_OversizeBody_RejectedBeforeCSRF proves the cap runs
@@ -1652,7 +1610,7 @@ func TestTwoMetricsRoutersInOneProcess(t *testing.T) {
 	)
 	third := &testEnv{
 		router: server.NewRouterForTest(
-			t, first.log, first.cfg, first.mw, first.hnd,
+			first.log.Get(), first.cfg, first.mw, first.hnd,
 		),
 	}