Correct the stop timeout comments on the archive writers

The comments on the engine's stop and on the timeout test gave false
reasons for leaving archive writers open when the stop budget runs
out. Closing them would wait for any write in progress, and a worker
still running would then open new writers that nothing closes, so
closing gains nothing over a kill.

Model: opus-5-5

README.md

@@ -968,15 +968,10 @@ 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 `webhooker.db` and every `events-*.db`, which
+A clean shutdown closes every database, which checkpoints and removes
-checkpoints and removes their sidecars; a killed or crashed instance
+its sidecars; a killed or crashed instance leaves them, and they must be
-leaves them, and they must be carried with the `.db`. **Archive
+carried with the `.db`. An archive the service has not opened since a
-databases are different**: their handle is not closed at shutdown, so
+crash keeps that crash's sidecars, even across a later clean stop.
-`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
@@ -1051,10 +1046,9 @@ 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.
+20 KB `.db` with no sidecars about a minute after its last write. A
-Shutdown is **not** on that list: the archive handle is not closed when
+clean stop closes it too. So either move `archive-{uuid}.db` together
-the service stops. So either move `archive-{uuid}.db` together with any
+with any `-wal`/`-shm` beside it, or wait until there are none.
-`-wal`/`-shm` beside it, or wait until there are none.
 
 ### Restore
 
@@ -1073,12 +1067,10 @@ the service stops. So either move `archive-{uuid}.db` together with any
    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 has none for `webhooker.db` or the `events-*.db`,
+   stop-and-copy set normally has none, because a clean stop closes
-   because a clean stop closes those and checkpoints their sidecars
+   every database and checkpoints its sidecars away; the exception is an
-   away — but it will normally have them for `archive-*.db`, whose
+   archive not opened since a crash. A copy salvaged from a crashed
-   handle stays open across shutdown, and those carry the archive's
+   instance has them for everything, and needs all of them.
-   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
@@ -3088,7 +3080,8 @@ 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`
+4. `delivery.Engine` — waits for its workers, then closes the archive
+   databases
 5. `healthcheck`
 6. `WebhookDBManager`
 7. the database close

internal/delivery/engine.go

@@ -362,6 +362,15 @@ 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")
 
@@ -376,6 +385,8 @@ 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,6 +2,8 @@ package delivery_test
 
 import (
 	"context"
+	"fmt"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -269,3 +271,88 @@ 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

@@ -1166,6 +1166,10 @@ func TestIsForwardableHeader(t *testing.T) {
 	assert.False(t,
 		delivery.ExportIsForwardableHeader("Content-Length"),
 	)
+
+	assert.False(t,
+		delivery.ExportIsForwardableHeader("Content-Type"),
+	)
 }
 
 func TestTruncate(t *testing.T) {
@@ -1247,6 +1251,81 @@ 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/redirect_test.go

@@ -339,10 +339,11 @@ 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 the delivery path's own two are deliberately
+// is not in the set, and neither is the inbound Content-Type, because
-// excluded: Content-Type describes the body, which a 307 carries
+// it is not forwarded. Two more are deliberately excluded: a
-// across hosts, and the inbound User-Agent every real sender supplies
+// Content-Type configured on the target describes the body, which a
-// is overwritten before the request goes out.
+// 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()
 
@@ -371,6 +372,7 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 		&delivery.HTTPTargetConfig{
 			Headers: map[string]string{
 				probeHeaderName: probeHeaderValue,
+				"Content-Type":  testContentType,
 			},
 		},
 	)
@@ -378,7 +380,11 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 	assert.Equal(t,
 		[]string{probeHeaderName, inboundHeaderName}, names,
 		"both header classes are reported, and only those: "+
-			"Host is never forwarded, Content-Type and "+
+			"Host and the inbound Content-Type are never "+
-			"User-Agent are the delivery path's own",
+			"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",
 	)
 }

internal/delivery/target_database.go

@@ -277,6 +277,24 @@ 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,6 +1,7 @@
 package delivery_test
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"net/http"
@@ -361,3 +362,46 @@ 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,10 +11,11 @@ import (
 	"sneak.berlin/go/webhooker/internal/delivery"
 )
 
-// Literals these tests repeat, named so that the header name and the
+// Literals these tests repeat, named so that the header names 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

@@ -541,6 +541,11 @@ 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
 	}
@@ -553,6 +558,10 @@ 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,
@@ -573,10 +582,10 @@ func applyRequestHeaders(
 
 	req.Header.Set("User-Agent", "webhooker/1.0")
 
-	// Content-Type describes the body being sent rather than the
+	// A Content-Type configured on the target describes the body
-	// sender, and the delivery path sets it from the event itself.
+	// being sent rather than the sender. A 307/308 preserves the
-	// A 307/308 preserves the body across hosts, so stripping it
+	// body across hosts, so stripping it would send that body
-	// would send that body untyped.
+	// untyped.
 	delete(originScoped, "Content-Type")
 
 	// User-Agent is overwritten just above, so an inbound one never