Try every event database file on delete and say which was left (closes #275)

DeleteDB stopped at the first file it could not remove, and the
webhook delete handler logged one message for every failure. Once
the database file itself is gone, its events are lost, and telling
the operator only that a file was left behind suggests otherwise.

DeleteDB now tries the database file and both sidecars, and its
error wraps ErrEventDBNotRemoved when the database file is still on
disk, or ErrSidecarNotRemoved when only a -wal or -shm sidecar is
left. The handler logs a different message for each.

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) |
@@ -2942,8 +2942,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

cmd/webhooker/main_test.go

@@ -14,7 +14,6 @@ import (
 
 	"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 +36,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()
@@ -75,7 +73,6 @@ func freePort(t *testing.T) int {
 // 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/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/database/webhook_db_manager.go

@@ -33,6 +33,19 @@ var errInvalidCachedDBType = errors.New(
 	"invalid cached database type",
 )
 
+// ErrEventDBNotRemoved is in DeleteDB's error when the event
+// database file itself could not be removed: it is still on disk.
+var ErrEventDBNotRemoved = errors.New(
+	"event database file not removed",
+)
+
+// ErrSidecarNotRemoved is in DeleteDB's error when the event
+// database file was removed, so its events are gone, but its -wal
+// or -shm sidecar could not be.
+var ErrSidecarNotRemoved = errors.New(
+	"event database file removed, but a -wal or -shm sidecar was not",
+)
+
 // WebhookDBManager manages per-webhook SQLite database files
 // for event storage. Each webhook gets its own dedicated
 // database containing Events, Deliveries, DeliveryResults and the
@@ -151,7 +164,10 @@ func (m *WebhookDBManager) DBExists(
 }
 
 // DeleteDB closes the connection and deletes the database file
-// for a webhook. The file is permanently removed.
+// for a webhook, with its -wal and -shm sidecars. The files are
+// permanently removed. Each file is tried even when another could
+// not be removed, and the error wraps ErrEventDBNotRemoved or
+// ErrSidecarNotRemoved to say which was left, naming each file.
 func (m *WebhookDBManager) DeleteDB(
 	webhookID string,
 ) error {
@@ -170,16 +186,23 @@ func (m *WebhookDBManager) DeleteDB(
 		}
 	}
 
-	// Delete the main DB file and WAL/SHM files
 	path := m.dbPath(webhookID)
-	for _, suffix := range []string{"", "-wal", "-shm"} {
-		err := os.Remove(path + suffix)
-		if err != nil && !os.IsNotExist(err) {
-			return fmt.Errorf(
-				"deleting webhook database file %s%s: %w",
-				path, suffix, err,
-			)
-		}
+
+	dbErr := removeFile(path)
+	sidecarErr := errors.Join(
+		removeFile(path+"-wal"),
+		removeFile(path+"-shm"),
+	)
+
+	if dbErr != nil {
+		return fmt.Errorf(
+			"%w: %w",
+			ErrEventDBNotRemoved, errors.Join(dbErr, sidecarErr),
+		)
+	}
+
+	if sidecarErr != nil {
+		return fmt.Errorf("%w: %w", ErrSidecarNotRemoved, sidecarErr)
 	}
 
 	m.log.Info(
@@ -190,6 +213,17 @@ func (m *WebhookDBManager) DeleteDB(
 	return nil
 }
 
+// removeFile removes path. A file that is already gone counts as
+// removed; the error from any other failure names the file.
+func removeFile(path string) error {
+	err := os.Remove(path)
+	if errors.Is(err, os.ErrNotExist) {
+		return nil
+	}
+
+	return err
+}
+
 // CloseAll closes all open per-webhook database connections.
 // Called during application shutdown.
 func (m *WebhookDBManager) CloseAll() error {

internal/database/webhook_db_manager_test.go

@@ -182,17 +182,91 @@ func TestWebhookDBManager_DeleteDB(t *testing.T) {
 	}
 	require.NoError(t, db.Create(event).Error)
 
+	// Under WAL, an open database that has been written to has both
+	// sidecars beside it.
+	dbPath := mgr.DBPath(webhookID)
+	require.FileExists(t, dbPath+"-wal")
+	require.FileExists(t, dbPath+"-shm")
+
 	// Delete the DB
 	require.NoError(t, mgr.DeleteDB(webhookID))
 
 	// File should no longer exist
 	assert.False(t, mgr.DBExists(webhookID))
 
-	// Verify the file is actually gone from disk
+	// Verify the files are actually gone from disk
+	assert.NoFileExists(t, dbPath)
+	assert.NoFileExists(t, dbPath+"-wal")
+	assert.NoFileExists(t, dbPath+"-shm")
+}
+
+// blockRemoval puts a non-empty directory at path, which os.Remove
+// cannot remove whoever runs the test, root included.
+func blockRemoval(t *testing.T, path string) {
+	t.Helper()
+
+	require.NoError(t, os.MkdirAll(filepath.Join(path, "keep"), 0o700))
+}
+
+// TestWebhookDBManager_DeleteDBKeepsDatabaseFile proves that when the
+// event database file cannot be removed, the error says so, and both
+// sidecars are still removed.
+func TestWebhookDBManager_DeleteDBKeepsDatabaseFile(t *testing.T) {
+	t.Parallel()
+
+	mgr, lc := setupTestWebhookDBManager(t)
+	ctx := context.Background()
+	require.NoError(t, lc.Start(ctx))
+
+	defer func() { require.NoError(t, lc.Stop(ctx)) }()
+
+	webhookID := uuid.New().String()
 	dbPath := mgr.DBPath(webhookID)
 
-	_, err = os.Stat(dbPath)
-	assert.True(t, os.IsNotExist(err))
+	blockRemoval(t, dbPath)
+	require.NoError(t, os.WriteFile(dbPath+"-wal", nil, 0o600))
+	require.NoError(t, os.WriteFile(dbPath+"-shm", nil, 0o600))
+
+	err := mgr.DeleteDB(webhookID)
+
+	require.ErrorIs(t, err, database.ErrEventDBNotRemoved)
+	require.NotErrorIs(t, err, database.ErrSidecarNotRemoved)
+	assert.Contains(t, err.Error(), dbPath)
+	assert.NoFileExists(t, dbPath+"-wal")
+	assert.NoFileExists(t, dbPath+"-shm")
+}
+
+// TestWebhookDBManager_DeleteDBKeepsSidecar proves that when the
+// event database file is removed but a sidecar is not, the error
+// says the database file is gone, and the other sidecar is still
+// removed.
+func TestWebhookDBManager_DeleteDBKeepsSidecar(t *testing.T) {
+	t.Parallel()
+
+	mgr, lc := setupTestWebhookDBManager(t)
+	ctx := context.Background()
+	require.NoError(t, lc.Start(ctx))
+
+	defer func() { require.NoError(t, lc.Stop(ctx)) }()
+
+	webhookID := uuid.New().String()
+	dbPath := mgr.DBPath(webhookID)
+
+	require.NoError(t, mgr.CreateDB(webhookID))
+	// Closing removes the sidecars, so the ones below are the only
+	// ones there.
+	require.NoError(t, mgr.CloseAll())
+
+	blockRemoval(t, dbPath+"-wal")
+	require.NoError(t, os.WriteFile(dbPath+"-shm", nil, 0o600))
+
+	err := mgr.DeleteDB(webhookID)
+
+	require.ErrorIs(t, err, database.ErrSidecarNotRemoved)
+	require.NotErrorIs(t, err, database.ErrEventDBNotRemoved)
+	assert.Contains(t, err.Error(), dbPath+"-wal")
+	assert.NoFileExists(t, dbPath)
+	assert.NoFileExists(t, dbPath+"-shm")
 }
 
 func TestWebhookDBManager_LazyCreation(t *testing.T) {

internal/delivery/archive_sweeper.go

@@ -45,13 +45,8 @@ type ArchiveSweeper struct {
 	eng      *Engine
 	log      *slog.Logger
 	interval time.Duration
-
-	// cancel needs no lock: fx calls the stop hook only after the
-	// start hook has returned, so stop never reads it while start
-	// is still setting it.
-	cancel context.CancelFunc
-
-	wg sync.WaitGroup
+	cancel   context.CancelFunc
+	wg       sync.WaitGroup
 }
 
 // NewArchiveSweeper creates the archive sweeper and registers
@@ -168,18 +163,10 @@ func (s *ArchiveSweeper) sweep(ctx context.Context) {
 	var targets []database.Target
 
 	err := s.db.DB().
-		WithContext(ctx).
 		Model(&database.Target{}).
 		Where("type = ?", database.TargetTypeDatabase).
 		Find(&targets).Error
 	if err != nil {
-		// The app stopping as a sweep starts cancels the listing.
-		// Stopping is not a failure, so it must not produce an
-		// error line.
-		if ctx.Err() != nil {
-			return
-		}
-
 		s.log.Error(
 			"archive sweep: failed to list database targets",
 			"error", err,

internal/delivery/archive_sweeper_test.go

@@ -1,11 +1,9 @@
 package delivery_test
 
 import (
-	"bytes"
 	"context"
 	"database/sql"
 	"fmt"
-	"log/slog"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -683,64 +681,6 @@ func TestArchiveSweep_ClosesHandleOfRegisteredWriter(
 	)
 }
 
-// TestArchiveSweep_ClosesHandleBeforeReopening proves the sweep
-// closes the handle it finds open before it reopens the file.
-// TestArchiveSweep_LeavesArchiveClosed cannot see this: without the
-// close, the reopen replaces the handle without closing it, the
-// sweep then closes only the new one, and one connection leaks per
-// archive per sweep.
-func TestArchiveSweep_ClosesHandleBeforeReopening(t *testing.T) {
-	t.Parallel()
-
-	path := filepath.Join(t.TempDir(), "archive.db")
-
-	w := delivery.NewExportArchiveWriter(
-		path, archiveTestLogger(), 0,
-	)
-
-	require.NoError(t, w.Open(time.Hour))
-
-	before, err := w.DB().DB()
-	require.NoError(t, err)
-
-	require.NoError(t, w.SweepExpired(time.Hour))
-
-	assert.Error(
-		t, before.PingContext(t.Context()),
-		"the handle open before the sweep must be closed by it",
-	)
-}
-
-// TestArchiveSweep_CancelledSweepLogsNoError proves a sweep whose
-// context is already cancelled, as when the app stops just as a
-// sweep starts, returns without an error line: stopping is not a
-// failure.
-func TestArchiveSweep_CancelledSweepLogsNoError(t *testing.T) {
-	t.Parallel()
-
-	env := setupArchiveTest(t)
-
-	var errorLines bytes.Buffer
-
-	sweeper := delivery.NewTestArchiveSweeper(
-		env.mainDB, env.eng,
-		slog.New(slog.NewTextHandler(
-			&errorLines,
-			&slog.HandlerOptions{Level: slog.LevelError},
-		)),
-	)
-
-	ctx, cancel := context.WithCancel(context.Background())
-	cancel()
-
-	sweeper.ExportSweep(ctx)
-
-	assert.Empty(
-		t, errorLines.String(),
-		"a cancelled sweep must not log at error level",
-	)
-}
-
 // TestArchiveSweep_NeverExpiryUntouched proves the sweep is a
 // no-op for the default retention policy, so archives with no
 // expiry (or the literal "never") behave exactly as before.

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/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/source_delete_test.go

@@ -1,8 +1,10 @@
 package handlers_test
 
 import (
+	"bytes"
 	"context"
 	"errors"
+	"log/slog"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -466,6 +468,66 @@ func TestHandleSourceDelete_RemovesConfigAndEventDatabase(
 	)
 }
 
+// TestHandleSourceDelete_LeftoverSidecar proves that when the event
+// database file is removed but a sidecar beside it is not, the
+// operator is told the events are gone, never that the event
+// database file is still there.
+func TestHandleSourceDelete_LeftoverSidecar(t *testing.T) {
+	t.Parallel()
+
+	var (
+		h    *handlers.Handlers
+		sess *session.Session
+		db   *database.Database
+		mgr  *database.WebhookDBManager
+	)
+
+	app := newTestApp(t, &h, &sess, &db, &mgr)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	logs := new(bytes.Buffer)
+	h.SetLogForTest(slog.New(slog.NewTextHandler(logs, nil)))
+
+	wh := seedWebhook(t, db)
+
+	require.NoError(t, mgr.CreateDB(wh.ID))
+	// Closing removes the sidecars, so the -wal below is the only
+	// one there.
+	require.NoError(t, mgr.CloseAll())
+
+	// A non-empty directory in the -wal file's place, which
+	// os.Remove cannot remove whoever runs the test.
+	eventDBPath := mgr.DBPath(wh.ID)
+	require.NoError(t, os.MkdirAll(
+		filepath.Join(eventDBPath+"-wal", "keep"), 0o700,
+	))
+
+	cookies := authenticatedCookies(
+		t, sess, deleteTestUserID, deleteTestUsername,
+	)
+
+	req := postRequest(
+		"/hook/"+wh.ID+"/delete",
+		cookies,
+		map[string]string{paramSourceID: wh.ID},
+	)
+	w := httptest.NewRecorder()
+
+	h.HandleSourceDelete().ServeHTTP(w, req)
+
+	assert.Equal(t, http.StatusInternalServerError, w.Code)
+	assert.NoFileExists(t, eventDBPath)
+	assert.Contains(t, logs.String(), "its events are gone")
+	assert.Contains(t, logs.String(), eventDBPath+"-wal")
+	assert.NotContains(
+		t, logs.String(), "event database file is still on disk",
+		"the events are gone, so the operator must not be told "+
+			"the event database file survived",
+	)
+}
+
 // TestHandleTargetDelete_EvictsThatTarget proves that deleting a
 // database target releases that target's archive writer and no
 // other: the webhook's other database target keeps its own.

internal/handlers/source_management.go

@@ -762,13 +762,21 @@ func (h *Handlers) deleteWebhookResources(
 	err = h.dbMgr.DeleteDB(webhook.ID)
 	if err != nil {
 		// The configuration is committed, so the webhook is gone,
-		// but its event database file is still on disk with
+		// but a file of its event database is still on disk with
 		// nothing referencing it. Report the failure rather than
 		// redirecting as though everything succeeded: the file
 		// needs removing by hand, and the logged error names it.
-		h.serverError(
-			w, r, "failed to delete webhook event database", err,
-		)
+		// When only a sidecar is left, the events are already
+		// gone, and the message must not suggest they survive.
+		msg := "webhook deleted, but its event database file is " +
+			"still on disk; remove it by hand"
+		if errors.Is(err, database.ErrSidecarNotRemoved) {
+			msg = "webhook deleted and its events are gone, but a " +
+				"-wal or -shm sidecar of its event database is " +
+				"still on disk; remove it by hand"
+		}
+
+		h.serverError(w, r, msg, err)
 
 		return
 	}

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/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/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_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,
 		),
 	}