Pin the HTTP target's unpinned error checks (closes #285)

Seven error checks in internal/delivery/target_http.go could be removed without any test noticing, among them withRetry's check on writing the delivery result, the branch that leaves a sent delivery retrying and recoverable when its bookkeeping write fails. Each now fails a test when removed. The "send succeeded" case starts from a tripped circuit breaker, so a probe whose send succeeds but whose result write fails must still close the breaker. The checks in remainingBackoff and backoffElapsed stay unpinned: removing them gives the same answer, and they state a rule a reader needs. Test change only.

Model: opus-5-5

internal/delivery/engine_integration_test.go

@@ -1425,6 +1425,32 @@ func TestDeliverHTTP_InvalidConfig(t *testing.T) {
 	)
 }
 
+// TestDeliverHTTP_InvalidConfigUnrecordedStaysPending: a delivery is
+// failed for an invalid config only once the reason is recorded.
+// Unrecorded, it stays pending, where the sweep finds it again.
+func TestDeliverHTTP_InvalidConfigUnrecordedStaysPending(t *testing.T) {
+	t.Parallel()
+
+	db := testWebhookDB(t)
+	e := testEngine(t, 1)
+
+	event, del := iSeedEventAndDelivery(
+		t, db, `{"config":"invalid"}`, "",
+	)
+
+	task, d := iHTTPTaskAndDelivery(
+		event, del, "bad-config", `not-json`, 0, 1,
+	)
+
+	require.NoError(t, db.Exec("drop table delivery_results").Error)
+
+	e.ExportDeliverHTTP(context.TODO(), db, d, task)
+
+	iAssertStatus(t, db, del.ID,
+		database.DeliveryStatusPending,
+	)
+}
+
 // --- Notify batching ---
 
 func TestNotify_MultipleTasks(t *testing.T) {

internal/delivery/engine_test.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"io"
 	"log/slog"
 	"net/http"
 	"net/http/httptest"
@@ -1056,6 +1057,21 @@ func TestParseHTTPConfig_MissingURL(t *testing.T) {
 	)
 }
 
+func TestParseHTTPConfig_Undecodable(t *testing.T) {
+	t.Parallel()
+
+	e := testEngine(t, 1)
+
+	_, err := e.ExportParseHTTPConfig(
+		`{"url":"https://example.com/hook","timeout":"soon"}`,
+	)
+
+	assert.Error(t, err,
+		"config that does not decode should return error, "+
+			"even when the part that did names a URL",
+	)
+}
+
 func TestScheduleRetry_SendsToRetryChannel(
 	t *testing.T,
 ) {
@@ -1241,6 +1257,33 @@ func TestDoHTTPRequest_ForwardsHeaders(t *testing.T) {
 	)
 }
 
+// A response that ends before the length it announced is an error, not
+// a short body.
+func TestDoHTTPRequest_CutShortResponseIsAnError(t *testing.T) {
+	t.Parallel()
+
+	ts := httptest.NewServer(
+		http.HandlerFunc(
+			func(w http.ResponseWriter, _ *http.Request) {
+				w.Header().Set("Content-Length", "100")
+				_, _ = w.Write([]byte("cut short"))
+			},
+		),
+	)
+	defer ts.Close()
+
+	e := testEngine(t, 1)
+
+	_, body, _, err := e.ExportDoHTTPRequest(
+		context.TODO(),
+		&delivery.HTTPTargetConfig{URL: ts.URL},
+		&database.Event{},
+	)
+
+	require.ErrorIs(t, err, io.ErrUnexpectedEOF)
+	assert.Empty(t, body)
+}
+
 // 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
@@ -1317,6 +1360,34 @@ func TestApplyRequestHeaders_SendsOneContentType(t *testing.T) {
 	}
 }
 
+// Stored inbound headers that do not decode forward nothing, not the
+// part of them that happened to decode.
+func TestApplyRequestHeaders_UndecodableInboundForwardsNothing(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	req, err := http.NewRequestWithContext(
+		context.Background(),
+		http.MethodPost,
+		"https://target.example.com/hook",
+		http.NoBody,
+	)
+	require.NoError(t, err)
+
+	names := delivery.ExportApplyRequestHeaders(
+		req,
+		&database.Event{
+			Headers: `{"X-Custom":["value1"],"X-Broken":"not a list"}`,
+		},
+		&delivery.HTTPTargetConfig{},
+		"webhooker/dev",
+	)
+
+	assert.Empty(t, names)
+	assert.Empty(t, req.Header.Get("X-Custom"))
+}
+
 func TestProcessDelivery_RoutesToCorrectHandler(
 	t *testing.T,
 ) {

internal/delivery/recovery_durability_test.go

@@ -376,3 +376,97 @@ func TestFailedResultWriteLeavesDeliveryRecoverable(
 		database.DeliveryStatusPending,
 	)
 }
+
+// TestFailedResultWriteWithRetriesLeavesDeliveryRecoverable is the same
+// rule for a target with retries: whatever the receiver answered, the
+// delivery stays pending and no retry is scheduled. The circuit breaker
+// still learns the answer, because it describes the target's health,
+// not the database's.
+func TestFailedResultWriteWithRetriesLeavesDeliveryRecoverable(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	// The "send succeeded" case starts with the breaker tripped open,
+	// so the delivery goes out as its probe and only a recorded
+	// success closes it again.
+	tests := []struct {
+		name        string
+		answer      int
+		tripped     bool
+		wantBreaker delivery.CircuitState
+	}{
+		{"send succeeded", http.StatusOK, true, delivery.CircuitClosed},
+		{"send failed", http.StatusBadGateway, false, delivery.CircuitOpen},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			s := newISetup(t)
+			targetID := uuid.New().String()
+
+			ts := httptest.NewServer(http.HandlerFunc(
+				func(w http.ResponseWriter, _ *http.Request) {
+					w.WriteHeader(tc.answer)
+				},
+			))
+			defer ts.Close()
+
+			event := iSeedEvent(
+				t, s.WebhookDB, s.WebhookID, `{"unwritable":true}`,
+			)
+
+			d := iSeedDelivery(
+				t, s.WebhookDB, event.ID, targetID,
+				database.DeliveryStatusPending,
+			)
+
+			require.NoError(
+				t,
+				s.WebhookDB.Exec("drop table delivery_results").Error,
+			)
+
+			// A single failure opens this breaker, and with no
+			// cooldown an open breaker lets the next delivery
+			// through as a probe.
+			cb := delivery.NewTestCircuitBreaker(1, 0)
+			if tc.tripped {
+				cb.RecordFailure()
+			}
+
+			s.Engine.ExportSetCircuitBreaker(targetID, cb)
+
+			full := &database.Delivery{
+				EventID:  event.ID,
+				TargetID: targetID,
+				Status:   database.DeliveryStatusPending,
+				Event:    event,
+				Target: database.Target{
+					Name:       "unwritable",
+					Type:       database.TargetTypeHTTP,
+					Config:     iHTTPConfig(ts.URL),
+					MaxRetries: 3,
+				},
+			}
+			full.ID = d.ID
+
+			sched := &recordingScheduler{}
+
+			s.Engine.ExportDeliverHTTPWithScheduler(
+				context.Background(), s.WebhookDB, full,
+				&delivery.Task{
+					DeliveryID: d.ID,
+					TargetID:   targetID,
+					AttemptNum: 1,
+				},
+				sched,
+			)
+
+			iAssertStatus(t, s.WebhookDB, d.ID, database.DeliveryStatusPending)
+			assert.Empty(t, sched.delays, "no retry may be scheduled")
+			assert.Equal(t, tc.wantBreaker, cb.State())
+		})
+	}
+}

internal/delivery/url_mask_test.go

@@ -179,6 +179,27 @@ func TestDoHTTPRequest_TransportErrorMasksURL(t *testing.T) {
 	)
 }
 
+// TestDoHTTPRequest_UnparsableURLIsMasked is the same for an HTTP
+// target URL that no request can be built from.
+func TestDoHTTPRequest_UnparsableURLIsMasked(t *testing.T) {
+	t.Parallel()
+
+	e := testEngine(t, 1)
+
+	statusCode, _, _, reqErr := e.ExportDoHTTPRequest(
+		context.TODO(),
+		&delivery.HTTPTargetConfig{
+			URL: "https://hooks.example.com" + maskSecretPath + "\n",
+		},
+		&database.Event{},
+	)
+	require.Error(t, reqErr)
+	assert.Zero(t, statusCode)
+
+	assertNoCredential(t, reqErr.Error())
+	assert.Contains(t, reqErr.Error(), "invalid control character")
+}
+
 // TestValidateTargetURL_UnparsableURLIsMasked proves the SSRF
 // validator's error does not carry the submitted URL, which
 // the handler both logs and shows.