Re-read a delivery before failing it for a missing target

failMissingTarget failed the delivery as the batch had read it, so a
delivery a worker sent and let go between the batch read and the
ownership check could end failed after a successful attempt. It now
reads the row once it owns the delivery and fails it only if the
status is still the one the batch read, as processNewTask does. A row
that cannot be read is left alone.

TestSweepPending_TargetDeleted now checks that the first sweep already
queues the healthy delivery and the second does not queue it again.

Model: opus-5-5

README.md

@@ -157,19 +157,6 @@ private and reserved ranges — RFC 1918, loopback, CGNAT, link-local and
 the rest — are refused, which stops a target from being used to make
 webhooker probe the network it sits in.
 
-Besides the private and reserved ranges, the default blocklist refuses
-public cloud metadata addresses: currently only `168.63.129.16`, Azure's
-WireServer, which serves an Azure VM its credentials. Because it is a
-public address, listing it in `ALLOWED_EGRESS_CIDRS` reopens it.
-
-That is all the default blocklist covers: private and reserved space,
-plus public addresses that serve cloud credentials. A cloud provider's
-other services on public addresses are not refused — IBM Cloud's
-`161.26.0.0/16` and `166.8.0.0/14`, for example, which carry its DNS
-resolvers, time servers and package mirrors. They serve no credentials,
-reaching them can be a legitimate delivery, and every cloud has some, so
-a partial list would promise coverage it does not give.
-
 That default is also inconvenient for the thing webhooker is mostly
 for: taking a public webhook and forwarding it to something on your own
 network. A container on the same Docker network, a box on `10.x`, a
@@ -208,16 +195,15 @@ Two things this setting cannot do:
   the list is always an allowlist; an empty list (the default) means
   every private and reserved range stays refused. Note that
   `0.0.0.0/0` gets you most of the way there anyway, per above.
-- **It cannot open link-local, or a cloud metadata endpoint at a
-  non-public address that discloses credentials or user data.** An
-  address is on the list below when it is not a public address and both
-  of these hold: the provider fixes it, so it cannot collide with
-  anything you run; and reaching it hands out credentials, user data or
-  bootstrap material. Those stay blocked no matter what you list,
-  including when you list them outright or list a supernet such as
-  `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as best
-  effort rather than a guarantee — it is a hand-maintained list and the
-  caveat below the table applies:
+- **It cannot open link-local, or a cloud metadata endpoint that
+  discloses credentials or user data.** An address is on the list below
+  when both of these hold: the provider fixes it, so it cannot collide
+  with anything you run; and reaching it hands out credentials, user
+  data or bootstrap material. Those stay blocked no matter what you
+  list, including when you list them outright or list a supernet such
+  as `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as
+  best effort rather than a guarantee — it is a hand-maintained list
+  and the caveat below the table applies:
 
   | Blocked unconditionally | What it is |
   | ----------------------- | ---------- |
@@ -256,8 +242,7 @@ Two things this setting cannot do:
   encodings, which the default blocklist does not match. A publicly
   routable metadata address is not listed here, because nothing on this
   list can be reopened and blocking one that way would leave you no
-  escape hatch at all; Azure's `168.63.129.16` is refused by the default
-  blocklist instead, as described above.
+  escape hatch at all.
 
   This list is not exhaustive of every cloud's metadata address — if
   yours is not here, do not allowlist the block that contains it.
@@ -983,10 +968,15 @@ scratch file**: it holds committed transactions that are not yet in the
 have no readable schema at all. `-shm` is regenerable, but there is no
 reason to separate the two — copy the directory and you have them.
 
-A clean shutdown closes every database, which checkpoints and removes
-its sidecars; a killed or crashed instance leaves them, and they must be
-carried with the `.db`. An archive the service has not opened since a
-crash keeps that crash's sidecars, even across a later clean stop.
+A clean shutdown closes `webhooker.db` and every `events-*.db`, which
+checkpoints and removes their sidecars; a killed or crashed instance
+leaves them, and they must be carried with the `.db`. **Archive
+databases are different**: their handle is not closed at shutdown, so
+`archive-*.db-wal` and `-shm` normally survive a clean stop and the
+`-wal` can hold every row the archive has. Measured on a stopped
+instance: `archive-….db` 4096 bytes with no table, its `-wal` 157 KB
+holding all 8 archived events. Copying `DATA_DIR` in full is what makes
+this a non-issue; copying `.db` files out of it by name is not.
 
 Configuration is **not** in `DATA_DIR` — it comes from the environment
 and from a `.env` file read out of the process working directory. Back
@@ -1061,9 +1051,10 @@ The file becomes self-contained again when the handle closes, which
 happens on the next write past the debounce window, when the connection
 pool retires the idle connection (about a minute after the last write),
 or at the idle archive sweep — measured, the same file was a complete
-20 KB `.db` with no sidecars about a minute after its last write. A
-clean stop closes it too. So either move `archive-{uuid}.db` together
-with any `-wal`/`-shm` beside it, or wait until there are none.
+20 KB `.db` with no sidecars about a minute after its last write.
+Shutdown is **not** on that list: the archive handle is not closed when
+the service stops. So either move `archive-{uuid}.db` together with any
+`-wal`/`-shm` beside it, or wait until there are none.
 
 ### Restore
 
@@ -1082,10 +1073,12 @@ with any `-wal`/`-shm` beside it, or wait until there are none.
    They are part of the database, and dropping a `-wal` silently
    discards every transaction it still holds. An `.backup` set will not
    contain any: it writes a single consolidated file per database. A
-   stop-and-copy set normally has none, because a clean stop closes
-   every database and checkpoints its sidecars away; the exception is an
-   archive not opened since a crash. A copy salvaged from a crashed
-   instance has them for everything, and needs all of them.
+   stop-and-copy set has none for `webhooker.db` or the `events-*.db`,
+   because a clean stop closes those and checkpoints their sidecars
+   away — but it will normally have them for `archive-*.db`, whose
+   handle stays open across shutdown, and those carry the archive's
+   rows. A copy salvaged from a crashed instance has them for
+   everything, and needs all of them.
 
 4. **Fix ownership.** The container runs as the non-root `webhooker`
    user, UID 1000 / GID 1000. Restored files must be owned by (or
@@ -2058,7 +2051,7 @@ rescans the database anyway).
 | ----------- | -------- |
 | **Closed**  | Normal operation. Deliveries flow through. Consecutive failures are counted. |
 | **Open**    | Target appears down. Deliveries are skipped and rescheduled for after the cooldown. |
-| **Half-Open** | Cooldown expired. One probe delivery is allowed to test if the target has recovered. Other deliveries are rescheduled for one whole cooldown later. |
+| **Half-Open** | Cooldown expired. One probe delivery is allowed to test if the target has recovered. |
 
 **Transitions:**
 
@@ -2096,9 +2089,7 @@ operations), and log targets (stdout) do not use circuit breakers.
 When a circuit is open and a new delivery arrives, the engine marks the
 delivery as `retrying` and schedules a retry timer for after the
 remaining cooldown period. This ensures no deliveries are lost — they're
-just delayed until the target is healthy again. A delivery already in
-`retrying` keeps that status without another database write each time
-the breaker turns it away.
+just delayed until the target is healthy again.
 
 ### Metrics
 
@@ -2112,7 +2103,7 @@ arriving and being stored, they are just not getting anywhere.
 | Metric | Type | Meaning |
 | ------ | ---- | ------- |
 | `webhooker_events_received_total` | counter | Events received and durably stored. Compare against the delivery counters on one dashboard |
-| `webhooker_delivery_attempts_total` | counter | Delivery attempts actually dispatched to a target. A delivery a circuit breaker refused is not one: it is counted as a retry instead, but only when the refusal moves it into `retrying` |
+| `webhooker_delivery_attempts_total` | counter | Delivery attempts actually dispatched to a target. A delivery an open circuit breaker refused is not one: it is counted as a retry instead |
 | `webhooker_deliveries_succeeded_total` | counter | Deliveries that reached `delivered` |
 | `webhooker_deliveries_failed_total` | counter | Deliveries that failed terminally and will not be retried |
 | `webhooker_delivery_retries_total` | counter | Deliveries put back into `retrying` |
@@ -3095,8 +3086,7 @@ each hook. The order, read off the fx stop-hook log:
 3. `server` — the HTTP drain, bounded separately by
    `server.ShutdownTimeout` (**3 seconds**), then a Sentry flush if
    `SENTRY_DSN` is set
-4. `delivery.Engine` — waits for its workers, then closes the archive
-   databases
+4. `delivery.Engine`
 5. `healthcheck`
 6. `WebhookDBManager`
 7. the database close

internal/config/config.go

@@ -192,10 +192,9 @@ type Config struct {
 	// alwaysBlockedNetworks stays blocked no matter what is listed
 	// here. That set is link-local plus the cloud metadata
 	// endpoints outside it that disclose credentials or user data
-	// at a provider-fixed, non-public address; it is not
-	// exhaustive of every cloud's metadata address. See
-	// alwaysBlockedNetworks for the authoritative list and the
-	// criterion it is built from.
+	// at a provider-fixed address; it is not exhaustive of every
+	// cloud's metadata address. See alwaysBlockedNetworks for the
+	// authoritative list and the criterion it is built from.
 	AllowedEgressCIDRs []netip.Prefix
 
 	params *ConfigParams
@@ -747,14 +746,12 @@ func (c *Config) warnEgressAllowlist(log *slog.Logger) {
 
 	log.Warn(
 		"ALLOWED_EGRESS_CIDRS lets delivery targets reach these "+
-			"otherwise-blocked networks. Anyone who can create a "+
-			"delivery target can now make this process issue "+
-			"requests into them, and read back the response. Only "+
-			"the addresses the README lists as blocked "+
-			"unconditionally stay blocked regardless of what is "+
-			"listed here; a public cloud metadata address such as "+
-			"168.63.129.16 is reachable once it, or a block "+
-			"covering it, is listed.",
+			"otherwise-blocked private/reserved networks. Anyone "+
+			"who can create a delivery target can now make this "+
+			"process issue requests into them, and read back the "+
+			"response. Link-local and the known cloud instance "+
+			"metadata endpoints outside it stay blocked "+
+			"regardless of what is listed here.",
 		"allowedEgressCIDRs",
 		strings.Join(PrefixStrings(c.AllowedEgressCIDRs), ","),
 	)

internal/config/config_test.go

@@ -834,13 +834,12 @@ func TestEgressAllowlistWarning(t *testing.T) {
 			// to be able to read back which networks are open.
 			assert.Contains(t, logged, "10.0.0.0/8")
 			assert.Contains(t, logged, "127.0.0.0/8")
-			// What stays shut is the whole unconditional set, not
-			// link-local alone; a public metadata address is not in
-			// it, so a listed block covering it opens it.
-			assert.Contains(t, logged, "blocked unconditionally")
-			assert.Contains(t, logged, "168.63.129.16 is reachable")
-			// The listed blocks need not be private or reserved.
-			assert.NotContains(t, logged, "private/reserved")
+			// What stays shut. Asserted on the clause naming the
+			// wider set rather than on "Link-local" alone, so the
+			// string cannot narrow back to link-local only while
+			// the always-blocked set covers ULA, CGNAT and two
+			// public metadata addresses as well.
+			assert.Contains(t, logged, "metadata endpoints outside it")
 		})
 	}
 }

internal/delivery/circuit_breaker.go

@@ -76,20 +76,12 @@ func (cb *CircuitBreaker) Allow() bool {
 	}
 }
 
-// CooldownRemaining returns how long a delivery that Allow refused
-// should wait before it is tried again. Closed, it returns zero.
-// Open, it returns what is left of the cooldown, or zero once that
-// has passed. Half-open, it returns the whole cooldown: the one
-// probe delivery is still in flight, and if it fails the circuit
-// reopens for that long.
+// CooldownRemaining returns how much time is left before
+// an open circuit transitions to half-open.
 func (cb *CircuitBreaker) CooldownRemaining() time.Duration {
 	cb.mu.Lock()
 	defer cb.mu.Unlock()
 
-	if cb.state == CircuitHalfOpen {
-		return cb.cooldown
-	}
-
 	if cb.state != CircuitOpen {
 		return 0
 	}

internal/delivery/circuit_breaker_test.go

@@ -267,7 +267,7 @@ func TestCircuitBreaker_CooldownRemaining_ClosedReturnsZero(
 	)
 }
 
-func TestCircuitBreaker_CooldownRemaining_HalfOpenReturnsCooldown(
+func TestCircuitBreaker_CooldownRemaining_HalfOpenReturnsZero(
 	t *testing.T,
 ) {
 	t.Parallel()
@@ -282,11 +282,9 @@ func TestCircuitBreaker_CooldownRemaining_HalfOpenReturnsCooldown(
 
 	require.True(t, cb.Allow())
 
-	// The cooldown newShortCooldownCB gives the breaker.
-	assert.Equal(t, 50*time.Millisecond,
+	assert.Equal(t, time.Duration(0),
 		cb.CooldownRemaining(),
-		"a delivery refused while half-open should wait "+
-			"a whole cooldown",
+		"half-open circuit should have zero cooldown remaining",
 	)
 }
 

internal/delivery/engine.go

@@ -362,15 +362,6 @@ func (e *Engine) start() {
 // stop cancels the worker pool's context and waits for the pool
 // to drain, bounded by the stop hook's context: a wedged worker
 // must not hang the process past fx's stop timeout.
-//
-// Once the pool has drained it closes the archive writers, so a
-// clean stop leaves no archive -wal behind. Nothing else holds a
-// writer for long by then: the archive sweeper stops before the
-// engine, and deleting a webhook only closes one. If the pool did
-// not drain in time, the writers are left open, as a kill would
-// leave them. Closing them would wait for any write in progress,
-// and a worker still running would then open new writers that
-// nothing closes, so it gains nothing over a kill.
 func (e *Engine) stop(ctx context.Context) error {
 	e.log.Info("delivery engine stopping")
 
@@ -385,8 +376,6 @@ func (e *Engine) stop(ctx context.Context) error {
 		return err
 	}
 
-	e.dbTarget.evictAll()
-
 	e.log.Info("delivery engine stopped")
 
 	return nil

internal/delivery/engine_lifecycle_test.go

@@ -2,8 +2,6 @@ package delivery_test
 
 import (
 	"context"
-	"fmt"
-	"path/filepath"
 	"testing"
 	"time"
 
@@ -271,88 +269,3 @@ func TestEngine_StopHookHonoursStopTimeout(t *testing.T) {
 
 	requireStopHookExpires(t, lc.hooks[0], "delivery engine")
 }
-
-// deliverToArchive runs one delivery to a database target through
-// the running engine and returns the webhook's archive file path.
-// The archive writer holds the file open afterwards.
-func deliverToArchive(t *testing.T, s iSetup) string {
-	t.Helper()
-
-	deliveryID, task := seedLogTask(t, s)
-	task.TargetType = database.TargetTypeDatabase
-
-	s.Engine.Notify([]delivery.Task{task})
-
-	iWaitForDelivered(t, s.WebhookDB, deliveryID)
-
-	return filepath.Join(
-		filepath.Dir(s.DBMgr.DBPath(s.WebhookID)),
-		fmt.Sprintf("archive-%s.db", s.WebhookID),
-	)
-}
-
-// TestEngine_StopHookClosesArchives is the regression test for an
-// archive split across two files by a clean stop. The engine never
-// closed its archive writers, so after a stop the archived rows
-// could sit in archive-{id}.db-wal while archive-{id}.db held no
-// table at all, and copying the .db on its own gave an empty
-// database.
-func TestEngine_StopHookClosesArchives(t *testing.T) {
-	t.Parallel()
-
-	s := newISetup(t)
-
-	lc := startEngineViaHook(t, s.Engine)
-
-	path := deliverToArchive(t, s)
-	require.FileExists(
-		t, path+"-wal",
-		"an open archive should have a -wal for the stop to remove",
-	)
-
-	require.NoError(t, lc.hooks[0].OnStop(context.Background()))
-
-	wals, err := filepath.Glob(
-		filepath.Join(filepath.Dir(path), "archive-*.db-wal"),
-	)
-	require.NoError(t, err)
-	require.Empty(
-		t, wals, "a clean stop must leave no archive -wal behind",
-	)
-
-	// With no -wal beside it, the row can only be in the .db.
-	count, err := countArchivedRows(path)
-	require.NoError(t, err)
-	require.Equal(t, int64(1), count)
-}
-
-// TestEngine_StopHookTimeoutLeavesArchivesOpen covers a stop whose
-// budget runs out while a worker is still running. The archive
-// writers are left open, as a kill would leave them: closing them
-// would wait for any write in progress, and that worker would then
-// open new writers that nothing closes.
-func TestEngine_StopHookTimeoutLeavesArchivesOpen(t *testing.T) {
-	t.Parallel()
-
-	s := newISetup(t)
-
-	lc := startEngineViaHook(t, s.Engine)
-
-	deliverToArchive(t, s)
-
-	release := make(chan struct{})
-
-	t.Cleanup(func() {
-		close(release)
-		s.Engine.EvictWebhook(s.WebhookID)
-	})
-
-	s.Engine.ExportWedgeWorker(release)
-
-	requireStopHookExpires(t, lc.hooks[0], "delivery engine")
-
-	require.True(
-		t, s.Engine.ExportArchiveHandleOpen(s.WebhookID),
-		"a stop that timed out must not close archive writers",
-	)
-}

internal/delivery/engine_test.go

@@ -17,7 +17,6 @@ import (
 	"time"
 
 	"github.com/google/uuid"
-	"github.com/prometheus/client_golang/prometheus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gorm.io/driver/sqlite"
@@ -25,7 +24,6 @@ import (
 	_ "modernc.org/sqlite"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/delivery"
-	"sneak.berlin/go/webhooker/internal/metrics"
 )
 
 // testContentType is the event content type used in tests.
@@ -896,100 +894,6 @@ func TestDeliverHTTP_CircuitBreakerBlocks(t *testing.T) {
 	)
 }
 
-// recordingScheduler keeps the delay of every retry it is asked to
-// schedule, and schedules nothing.
-type recordingScheduler struct {
-	delays []time.Duration
-}
-
-func (s *recordingScheduler) ScheduleRetry(
-	_ delivery.Task, delay time.Duration,
-) {
-	s.delays = append(s.delays, delay)
-}
-
-// TestDeliverHTTP_HalfOpenBreakerDelaysQueuedTasks proves that while a
-// half-open breaker's one probe delivery is in flight, every other task
-// for the target is put back with a whole cooldown as its delay rather
-// than none, and that its status is written the first time the breaker
-// turns it away and not on each pass after that.
-func TestDeliverHTTP_HalfOpenBreakerDelaysQueuedTasks(t *testing.T) {
-	t.Parallel()
-
-	db := testWebhookDB(t)
-	e := testEngine(t, 1)
-
-	// Every write of retrying moves the retry counter, so on a registry
-	// this test owns the counter is the number of those writes.
-	reg := prometheus.NewRegistry()
-	e.ExportSetMetrics(metrics.New(reg))
-
-	targetID := uuid.New().String()
-	cb := newShortCooldownCB(t)
-	e.ExportSetCircuitBreaker(targetID, cb)
-
-	for range delivery.ExportDefaultFailureThreshold {
-		cb.RecordFailure()
-	}
-
-	time.Sleep(60 * time.Millisecond)
-
-	require.True(t, cb.Allow(), "the probe delivery should go through")
-	require.Equal(t, delivery.CircuitHalfOpen, cb.State())
-
-	cfg := newHTTPTargetConfig(
-		"http://will-not-be-called.invalid",
-	)
-	sched := &recordingScheduler{}
-
-	const queued, passes = 3, 4
-
-	for range queued {
-		event := seedEvent(t, db, `{"cb":"half-open"}`)
-		dlv := seedDelivery(
-			t, db, event.ID, targetID,
-			database.DeliveryStatusPending,
-		)
-
-		for range passes {
-			// Each pass starts from the stored row, as a retry does.
-			var row database.Delivery
-
-			require.NoError(t, db.First(
-				&row, "id = ?", dlv.ID,
-			).Error)
-
-			fix := buildHTTPFixture(
-				row, event, targetID,
-				"test-cb-half-open", cfg, 5, 1,
-			)
-
-			e.ExportDeliverHTTPWithScheduler(
-				context.TODO(), db, fix.Delivery, fix.Task, sched,
-			)
-		}
-
-		assertDeliveryStatus(t, db, dlv.ID,
-			database.DeliveryStatusRetrying,
-		)
-	}
-
-	require.Len(t, sched.delays, queued*passes)
-
-	for _, delay := range sched.delays {
-		// The cooldown newShortCooldownCB gives the breaker.
-		assert.Equal(t, 50*time.Millisecond, delay,
-			"a task turned away while half-open should wait "+
-				"a whole cooldown",
-		)
-	}
-
-	assert.InDelta(t, float64(queued),
-		mCounter(t, reg, mRetries, mTypeHTTP), 0,
-		"status should be written once per task, not once per pass",
-	)
-}
-
 func TestGetCircuitBreaker_CreatesOnDemand(t *testing.T) {
 	t.Parallel()
 
@@ -1166,10 +1070,6 @@ func TestIsForwardableHeader(t *testing.T) {
 	assert.False(t,
 		delivery.ExportIsForwardableHeader("Content-Length"),
 	)
-
-	assert.False(t,
-		delivery.ExportIsForwardableHeader("Content-Type"),
-	)
 }
 
 func TestTruncate(t *testing.T) {
@@ -1251,81 +1151,6 @@ func TestDoHTTPRequest_ForwardsHeaders(t *testing.T) {
 	)
 }
 
-// The event's stored inbound headers carry the same Content-Type the
-// receiver saved as the event's ContentType, so a delivery could send
-// it twice. It must go out exactly once, with a Content-Type configured
-// on the target winning, then the event's ContentType.
-func TestApplyRequestHeaders_SendsOneContentType(t *testing.T) {
-	t.Parallel()
-
-	cases := map[string]struct {
-		inbound    string
-		event      string
-		configured string
-		want       []string
-	}{
-		"inbound and event agree": {
-			inbound: testContentType,
-			event:   testContentType,
-			want:    []string{testContentType},
-		},
-		"inbound and event disagree": {
-			inbound: "text/plain",
-			event:   testContentType,
-			want:    []string{testContentType},
-		},
-		"event has none": {
-			inbound: testContentType,
-			want:    nil,
-		},
-		"target configures its own": {
-			inbound:    testContentType,
-			event:      testContentType,
-			configured: "application/xml",
-			want:       []string{"application/xml"},
-		},
-	}
-
-	for name, tc := range cases {
-		t.Run(name, func(t *testing.T) {
-			t.Parallel()
-
-			inbound, err := json.Marshal(map[string][]string{
-				headerContentType: {tc.inbound},
-			})
-			require.NoError(t, err)
-
-			cfg := &delivery.HTTPTargetConfig{}
-			if tc.configured != "" {
-				cfg.Headers = map[string]string{
-					headerContentType: tc.configured,
-				}
-			}
-
-			req, err := http.NewRequestWithContext(
-				context.Background(),
-				http.MethodPost,
-				"https://target.example.com/hook",
-				http.NoBody,
-			)
-			require.NoError(t, err)
-
-			delivery.ExportApplyRequestHeaders(
-				req,
-				&database.Event{
-					Headers:     string(inbound),
-					ContentType: tc.event,
-				},
-				cfg,
-			)
-
-			assert.Equal(t,
-				tc.want, req.Header.Values(headerContentType),
-			)
-		})
-	}
-}
-
 func TestProcessDelivery_RoutesToCorrectHandler(
 	t *testing.T,
 ) {

internal/delivery/export_test.go

@@ -101,19 +101,6 @@ func (e *Engine) ExportDeliverHTTP(
 	e.httpTarget.Deliver(ctx, webhookDB, d, task, e)
 }
 
-// ExportDeliverHTTPWithScheduler delivers via the http target, handing
-// any retry to sched instead of the engine, so a test can see the
-// delay each retry is given.
-func (e *Engine) ExportDeliverHTTPWithScheduler(
-	ctx context.Context,
-	webhookDB *gorm.DB,
-	d *database.Delivery,
-	task *Task,
-	sched Scheduler,
-) {
-	e.httpTarget.Deliver(ctx, webhookDB, d, task, sched)
-}
-
 // ExportDeliverDatabase delivers via the database target.
 func (e *Engine) ExportDeliverDatabase(
 	webhookDB *gorm.DB, d *database.Delivery,
@@ -192,14 +179,6 @@ func (e *Engine) ExportGetCircuitBreaker(
 	return e.httpTarget.getCircuitBreaker(targetID)
 }
 
-// ExportSetCircuitBreaker makes cb the http target's circuit breaker
-// for targetID, so a test can use one with a short cooldown.
-func (e *Engine) ExportSetCircuitBreaker(
-	targetID string, cb *CircuitBreaker,
-) {
-	e.httpTarget.circuitBreakers.Store(targetID, cb)
-}
-
 // ExportParseHTTPConfig exposes parseHTTPConfig.
 func (e *Engine) ExportParseHTTPConfig(
 	configJSON string,
@@ -352,21 +331,6 @@ func (e *Engine) ExportFailMissingTarget(
 	e.failMissingTarget(webhookDB, webhookID, d)
 }
 
-// ExportSendRecoveredDeliveries exposes sendRecoveredDeliveries, so a
-// test can hand it a target map that lacks a delivery's target.
-func (e *Engine) ExportSendRecoveredDeliveries(
-	ctx context.Context,
-	webhookDB *gorm.DB,
-	deliveries []database.Delivery,
-	webhookID string,
-	targetMap map[string]database.Target,
-	settled map[string]struct{},
-) {
-	e.sendRecoveredDeliveries(
-		ctx, webhookDB, deliveries, webhookID, targetMap, settled,
-	)
-}
-
 // ExportDeliveryCh returns the delivery channel.
 func (e *Engine) ExportDeliveryCh() chan Task {
 	return e.deliveryCh

internal/delivery/metrics_test.go

@@ -412,10 +412,9 @@ func TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt(
 
 	s.Engine.ExportProcessRetryTask(context.TODO(), &blocked)
 
-	// The breaker refused it: rescheduled without rewriting the
-	// retrying status it already had, so the retry counter did not
-	// move, and nothing was attempted or timed.
-	assert.InDelta(t, retriesBefore,
+	// The breaker refused it: rescheduled, so the retry counter
+	// moved, but nothing was attempted or timed.
+	assert.InDelta(t, retriesBefore+1,
 		mCounter(t, reg, mRetries, mTypeHTTP), 0)
 	assert.InDelta(t, threshold,
 		mCounter(t, reg, mAttempts, mTypeHTTP), 0)

internal/delivery/redirect_test.go

@@ -339,11 +339,10 @@ func TestRedirectPolicy_StopsAtHopCap(t *testing.T) {
 // The set the redirect policy strips is whatever the delivery path
 // actually put on the wire, so a header added to the forward set is
 // covered without a second edit. A header the event never carried
-// is not in the set, and neither is the inbound Content-Type, because
-// it is not forwarded. Two more are deliberately excluded: a
-// Content-Type configured on the target describes the body, which a
-// 307 carries across hosts, and the inbound User-Agent every real
-// sender supplies is overwritten before the request goes out.
+// is not in the set, and the delivery path's own two are deliberately
+// excluded: Content-Type describes the body, which a 307 carries
+// across hosts, and the inbound User-Agent every real sender supplies
+// is overwritten before the request goes out.
 func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 	t.Parallel()
 
@@ -372,7 +371,6 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 		&delivery.HTTPTargetConfig{
 			Headers: map[string]string{
 				probeHeaderName: probeHeaderValue,
-				"Content-Type":  testContentType,
 			},
 		},
 	)
@@ -380,11 +378,7 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 	assert.Equal(t,
 		[]string{probeHeaderName, inboundHeaderName}, names,
 		"both header classes are reported, and only those: "+
-			"Host and the inbound Content-Type are never "+
-			"forwarded, User-Agent is the delivery path's own",
-	)
-	assert.NotContains(t, names, "Content-Type",
-		"a Content-Type configured on the target must survive "+
-			"a cross-origin 307/308 with the body it describes",
+			"Host is never forwarded, Content-Type and "+
+			"User-Agent are the delivery path's own",
 	)
 }

internal/delivery/ssrf.go

@@ -26,7 +26,7 @@ var (
 		"hostname resolved to no IP addresses",
 	)
 	errBlockedIP = errors.New(
-		"blocked private, reserved or cloud metadata address",
+		"blocked private/reserved IP range",
 	)
 	errBlockedMetadata = errors.New(
 		"blocked link-local or cloud instance metadata " +
@@ -37,18 +37,11 @@ var (
 	)
 )
 
-// blockedNetworks is the default blocklist: the private and
-// reserved IP ranges, plus the public cloud metadata addresses,
-// that are blocked to prevent SSRF attacks. An operator can
-// permit specific blocks out of this set with
+// blockedNetworks contains all private/reserved IP ranges
+// that should be blocked to prevent SSRF attacks. An operator
+// can permit specific blocks out of this set with
 // ALLOWED_EGRESS_CIDRS; see Guard.
 //
-// A public address belongs here only if it serves cloud
-// credentials; a provider's other services on public addresses,
-// such as its DNS resolvers or package mirrors, stay out, since
-// reaching them can be legitimate and no list of them could be
-// complete.
-//
 //nolint:gochecknoglobals // package-level network list is appropriate here
 var blockedNetworks []*net.IPNet
 
@@ -129,8 +122,6 @@ func init() {
 		"::1/128",
 		"fc00::/7",
 		"fe80::/10",
-		// Azure WireServer, a public address that serves VM credentials.
-		"168.63.129.16/32",
 	})
 
 	// Every entry is named. The set must not grow or shrink
@@ -225,8 +216,8 @@ func matchesAny(networks []*net.IPNet, ip net.IP) bool {
 }
 
 // isBlockedIP checks whether an IP address falls within
-// the default blocklist, before any operator allowlist is
-// considered.
+// any blocked private/reserved network range, before any
+// operator allowlist is considered.
 func isBlockedIP(ip net.IP) bool {
 	return matchesAny(blockedNetworks, ip)
 }
@@ -329,7 +320,7 @@ func (g *Guard) allows(ip net.IP) bool {
 //
 //  1. alwaysBlockedNetworks is refused before the allowlist is
 //     consulted, so no configured CIDR reaches link-local or a
-//     cloud metadata endpoint at a non-public address.
+//     cloud instance metadata endpoint.
 //  2. The allowlist is consulted next, so a listed private
 //     network becomes reachable.
 //  3. Everything else keeps the default blocklist's answer.

internal/delivery/ssrf_allowlist_test.go

@@ -390,41 +390,6 @@ func TestGuardAllowlist_PublicUnaffected(t *testing.T) {
 	}
 }
 
-// TestGuardAllowlist_AzureWireServerReopenable covers Azure's
-// WireServer, a public address that serves VM credentials. The
-// default guard refuses it, but because it is public it sits in
-// the default blocklist rather than the unconditional set, so an
-// operator who lists it can reach it.
-func TestGuardAllowlist_AzureWireServerReopenable(t *testing.T) {
-	t.Parallel()
-
-	const wireServerIP = "168.63.129.16"
-
-	target := "http://" + wireServerIP + "/?comp=versions"
-
-	defaultGuard := delivery.NewTestGuard()
-
-	err := defaultGuard.ValidateTargetURL(context.Background(), target)
-	require.Error(t, err,
-		"WireServer must be refused with no allowlist set",
-	)
-	assert.NotContains(t, err.Error(), metadataRefusalClause,
-		"WireServer must be refused by the default blocklist, "+
-			"which an allowlist can override",
-	)
-
-	assertDialRefused(t, defaultGuard, target)
-
-	listed := delivery.NewTestGuard(
-		netip.MustParsePrefix(wireServerIP + "/32"),
-	)
-
-	assert.NoError(t,
-		listed.ValidateTargetURL(context.Background(), target),
-		"an operator who lists WireServer must be able to reach it",
-	)
-}
-
 // TestGuardCheckIP_BothPathsShareOneDecision asserts that the
 // validator and the dialer are not two policies that happen to
 // agree: both are defined in terms of checkIP, so the exported

internal/delivery/target_database.go

@@ -277,24 +277,6 @@ func (t *databaseTarget) evict(webhookID string) {
 	)
 }
 
-// evictAll evicts every cached archive writer, exactly as evict
-// does for one webhook. The engine calls it at shutdown, once its
-// workers have returned. Closing the last handle on an archive
-// moves the contents of its -wal into the .db and removes the
-// -wal, so a clean stop leaves each archive as a single file.
-func (t *databaseTarget) evictAll() {
-	t.mu.Lock()
-
-	writers := t.writers
-	t.writers = nil
-
-	t.mu.Unlock()
-
-	for _, w := range writers {
-		w.evict()
-	}
-}
-
 // sweepWebhook prunes one webhook's archive of rows older than
 // expiry, without requiring a write. It returns nil (nothing to
 // do) when the archive file does not exist, so a sweep never

internal/delivery/target_database_evict_test.go

@@ -1,7 +1,6 @@
 package delivery_test
 
 import (
-	"context"
 	"errors"
 	"fmt"
 	"net/http"
@@ -362,46 +361,3 @@ func TestEvictWebhook_LaterDeliveryRecreatesWriter(t *testing.T) {
 		"a later delivery should recreate the writer",
 	)
 }
-
-// TestEngineStop_WriteAfterStopIsRefused proves the engine's stop
-// closes each archive writer the way deleting its webhook does: a
-// write that reaches a writer after the stop is refused, reopens
-// nothing and adds no row.
-func TestEngineStop_WriteAfterStopIsRefused(t *testing.T) {
-	t.Parallel()
-
-	eng, _ := evictTestEngine(t)
-
-	webhookDB := testWebhookDB(t)
-	event := seedEvent(t, webhookDB, `{"archived":true}`)
-	d := seedDatabaseTargetDelivery(t, webhookDB, event, "")
-
-	eng.ExportDeliverDatabase(webhookDB, d)
-
-	w := eng.ExportArchiveWriterFor(event.WebhookID)
-	require.NotNil(t, w)
-	require.True(t, w.HandleOpen())
-
-	require.NoError(t, eng.ExportStop(context.Background()))
-
-	err := w.Write(evictTestRow("ev-after-stop"), 0)
-
-	require.ErrorIs(
-		t, err, delivery.ErrExportArchiveWriterEvicted,
-		"a write after the stop must be refused",
-	)
-	assert.False(
-		t, w.HandleOpen(),
-		"a refused write must not reopen the archive",
-	)
-	assert.False(
-		t, eng.ExportHasArchiveWriter(event.WebhookID),
-		"the stop should empty the registry",
-	)
-
-	count, err := countArchivedRows(w.Path())
-	require.NoError(t, err)
-	assert.Equal(
-		t, int64(1), count, "the refused row must not be written",
-	)
-}

internal/delivery/target_headers_test.go

@@ -11,11 +11,10 @@ import (
 	"sneak.berlin/go/webhooker/internal/delivery"
 )
 
-// Literals these tests repeat, named so that the header names and the
+// Literals these tests repeat, named so that the header name and the
 // keep-forever archive config each have one definition.
 const (
 	headerAuthorization = "Authorization"
-	headerContentType   = "Content-Type"
 	bearerValue         = "Bearer abc"
 	archiveConfigNever  = "{\"expiry\":\"never\"}"
 )

internal/delivery/target_http.go

@@ -197,14 +197,10 @@ func (c *httpCore) circuitBreakerBlock(
 		"cooldown_remaining", remaining,
 	)
 
-	// A delivery already at retrying is left as it is, so a task
-	// the breaker keeps turning away writes nothing each time.
-	if d.Status != database.DeliveryStatusRetrying {
 	c.eng.settleStatus(
 		webhookDB, d, d.Target.Type,
 		database.DeliveryStatusRetrying,
 	)
-	}
 
 	retryTask := *task
 	sched.ScheduleRetry(retryTask, remaining)
@@ -541,11 +537,6 @@ func isForwardableHeader(name string) bool {
 		"Upgrade", "Proxy-Authorization",
 		"Proxy-Connection", "Content-Length":
 		return false
-	case "Content-Type":
-		// applyRequestHeaders sets Content-Type itself. The receiver
-		// already stored this inbound value as the event's
-		// ContentType, so forwarding it too would send it twice.
-		return false
 	default:
 		return true
 	}
@@ -558,10 +549,6 @@ func isForwardableHeader(name string) bool {
 // policy strips exactly that set on a hop that leaves the origin,
 // so the forward set is decided here and only here — a header added
 // to it is covered off-origin without a second edit elsewhere.
-//
-// Content-Type goes out once: a Content-Type configured on the target
-// wins, otherwise the event's ContentType, otherwise none. The inbound
-// Content-Type in the event's headers is never forwarded.
 func applyRequestHeaders(
 	req *http.Request,
 	event *database.Event,
@@ -582,10 +569,10 @@ func applyRequestHeaders(
 
 	req.Header.Set("User-Agent", "webhooker/1.0")
 
-	// A Content-Type configured on the target describes the body
-	// being sent rather than the sender. A 307/308 preserves the
-	// body across hosts, so stripping it would send that body
-	// untyped.
+	// Content-Type describes the body being sent rather than the
+	// sender, and the delivery path sets it from the event itself.
+	// A 307/308 preserves the body across hosts, so stripping it
+	// would send that body untyped.
 	delete(originScoped, "Content-Type")
 
 	// User-Agent is overwritten just above, so an inbound one never

internal/delivery/terminal_state_test.go

@@ -696,53 +696,6 @@ func TestSweepPending_TargetDeleted(t *testing.T) {
 	assert.Contains(t, last.Error, "was deleted")
 }
 
-// TestSendRecoveredDeliveries_TargetMissingFromMap: the batch's target
-// map is empty when its query failed, so every delivery in the batch is
-// looked up on its own. A healthy one is sent to the target that lookup
-// finds.
-func TestSendRecoveredDeliveries_TargetMissingFromMap(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	s := newISetup(t)
-
-	targetID := uuid.New().String()
-
-	iCreateTarget(
-		t, s.MainDB, targetID, s.WebhookID, "found-on-lookup",
-		database.TargetTypeLog, "", 0,
-	)
-
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"map":"empty"}`,
-	)
-
-	d := iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusPending,
-	)
-
-	s.Engine.ExportSendRecoveredDeliveries(
-		context.Background(), s.WebhookDB,
-		[]database.Delivery{d}, s.WebhookID,
-		map[string]database.Target{}, nil,
-	)
-
-	tasks := fDrain(s.Engine)
-	require.Len(t, tasks, 1,
-		"the healthy delivery was not queued exactly once",
-	)
-	assert.Equal(t, d.ID, tasks[0].DeliveryID)
-	assert.Equal(t, targetID, tasks[0].TargetID)
-	assert.Equal(t, database.TargetTypeLog, tasks[0].TargetType)
-
-	iAssertStatus(
-		t, s.WebhookDB, d.ID,
-		database.DeliveryStatusPending,
-	)
-}
-
 // TestRecoverPending_TargetUnreadable_LeavesDeliveryAlone: a failed
 // read of the main database is not a deleted target. Restart recovery
 // holds every pending delivery of the webhook in one batch, so failing

internal/middleware/recoverer.go

@@ -133,11 +133,6 @@ func (w *recoverResponseWriter) Unwrap() http.ResponseWriter {
 // what the access log records and the metrics count, and outside the
 // sentryhttp handler, whose Repanic option depends on something
 // further out recovering what it re-raises.
-//
-// Unlike http.Error on its own, it deletes any Set-Cookie the handler
-// set before panicking, because a request that failed must not hand
-// the client a credential; every other header is left to http.Error.
-// See https://git.eeqj.de/sneak/webhooker/issues/193.
 func (s *Middleware) Recoverer() func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(
@@ -169,8 +164,6 @@ func (s *Middleware) Recoverer() func(http.Handler) http.Handler {
 					return
 				}
 
-				rw.Header().Del("Set-Cookie")
-
 				http.Error(
 					rw,
 					http.StatusText(

internal/middleware/recoverer_test.go

@@ -304,44 +304,16 @@ func TestRecovererRepanicsErrAbortHandler(t *testing.T) {
 	)
 }
 
-// TestRecovererDropsSetCookieFromTheRecovered500 covers a handler that
-// sets a cookie and a redirect target and then panics before sending
-// anything. A request that failed must not hand the client a
-// credential, so the 500 carries no cookie; Location is left alone.
-func TestRecovererDropsSetCookieFromTheRecovered500(t *testing.T) {
-	t.Parallel()
-
-	probe := newRecovererProbe(
-		t, false,
-		func(w http.ResponseWriter, _ *http.Request) {
-			w.Header().Set("Set-Cookie", "session=x")
-			w.Header().Set("Location", "/after")
-
-			panic(panicMarker)
-		},
-	)
-
-	resp, err := probe.get(t)
-	require.NoError(t, err)
-	require.NoError(t, resp.Body.Close())
-
-	assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)
-	assert.Empty(t, resp.Cookies())
-	assert.Equal(t, "/after", resp.Header.Get("Location"))
-}
-
 // TestRecovererKeepsAnAlreadyCommittedResponse covers a handler that
 // panics after sending its status. The bytes are already on the wire,
-// cookie included, so a second WriteHeader would change nothing the
-// client sees and would draw net/http's "superfluous
-// response.WriteHeader" report.
+// so a second WriteHeader would change nothing the client sees and
+// would draw net/http's "superfluous response.WriteHeader" report.
 func TestRecovererKeepsAnAlreadyCommittedResponse(t *testing.T) {
 	t.Parallel()
 
 	probe := newRecovererProbe(
 		t, false,
 		func(w http.ResponseWriter, _ *http.Request) {
-			w.Header().Set("Set-Cookie", "session=x")
 			w.WriteHeader(committedStatus)
 			_, _ = w.Write([]byte("partial"))
 
@@ -359,7 +331,6 @@ func TestRecovererKeepsAnAlreadyCommittedResponse(t *testing.T) {
 
 	assert.Equal(t, committedStatus, resp.StatusCode)
 	assert.Equal(t, "partial", string(body))
-	assert.Len(t, resp.Cookies(), 1)
 
 	record := probe.panicRecord(t)
 	assert.Equal(t, panicMarker, record["panic"])