Leave a delivery cut off by a stop for the next start (closes #527)

Stopping the engine cancels the context every http and slack request is
sent with. An attempt cut off that way was recorded as failed, which for
a target with no retries, or on a last retry, ended the delivery failed
and nothing sent it again.

The shared retry core now records nothing for a failed attempt when the
engine's context is cancelled: no result, no circuit breaker outcome, no
status change. The delivery stays pending or retrying and startup
recovery sends it again. A target's own timeout is still a failed
attempt. The README's Graceful shutdown bullet, Shutdown section and the
at-least-once note say so.

Model: opus-5-5

internal/delivery/engine_lifecycle_test.go

@@ -2,11 +2,16 @@ package delivery_test
 
 import (
 	"context"
+	"io"
+	"net/http"
+	"net/http/httptest"
 	"path/filepath"
+	"sync"
 	"testing"
 	"time"
 
 	"github.com/google/uuid"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/fx"
 	"gorm.io/gorm/clause"
@@ -272,6 +277,151 @@ func TestEngine_StopHookHonoursStopTimeout(t *testing.T) {
 	requireStopHookExpires(t, lc.hooks[0], "delivery engine")
 }
 
+// seedHeldDelivery gives the setup's webhook an http target with
+// maxRetries, pointed at a receiver that keeps every request open until
+// the client gives up on it, and seeds one delivery to that target at
+// status. It returns the task that sends attempt attemptNum of the
+// delivery, and a channel that is closed when a request reaches the
+// receiver.
+func seedHeldDelivery(
+	t *testing.T,
+	s iSetup,
+	maxRetries int,
+	status database.DeliveryStatus,
+	attemptNum int,
+) (delivery.Task, <-chan struct{}) {
+	t.Helper()
+
+	arrived := make(chan struct{})
+
+	var once sync.Once
+
+	ts := httptest.NewServer(http.HandlerFunc(
+		func(_ http.ResponseWriter, r *http.Request) {
+			// The server notices the client hanging up only once the
+			// body has been read; until then r.Context() never ends.
+			_, _ = io.Copy(io.Discard, r.Body)
+
+			once.Do(func() { close(arrived) })
+
+			<-r.Context().Done()
+		},
+	))
+	t.Cleanup(ts.Close)
+
+	cfg := iHTTPConfig(ts.URL)
+	targetID := uuid.New().String()
+
+	iCreateWebhook(t, s.MainDB, s.WebhookID, "hook")
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "held",
+		database.TargetTypeHTTP, cfg, maxRetries,
+	)
+
+	event := iSeedEvent(t, s.WebhookDB, s.WebhookID, `{"held":true}`)
+	d := iSeedDelivery(t, s.WebhookDB, event.ID, targetID, status)
+
+	bodyStr := event.Body
+
+	return iTask(
+		d, event, s.WebhookID, targetID, "held", cfg,
+		maxRetries, attemptNum, &bodyStr,
+	), arrived
+}
+
+// stopDuringAttempt waits until the receiver holds an attempt open, then
+// stops the engine through its real stop hook.
+func stopDuringAttempt(
+	t *testing.T, lc *recordingLifecycle, arrived <-chan struct{},
+) {
+	t.Helper()
+
+	select {
+	case <-arrived:
+	case <-time.After(5 * time.Second):
+		t.Fatal("the delivery attempt never reached the receiver")
+	}
+
+	require.NoError(t, lc.hooks[0].OnStop(context.Background()))
+}
+
+// requireRequeued runs restart recovery on the stopped engine and
+// requires it to queue task's attempt again on ch.
+func requireRequeued(
+	t *testing.T, s iSetup, ch <-chan delivery.Task, task delivery.Task,
+) {
+	t.Helper()
+
+	s.Engine.ExportRecoverWebhookDeliveries(
+		context.Background(), s.WebhookID,
+	)
+
+	select {
+	case got := <-ch:
+		assert.Equal(t, task.DeliveryID, got.DeliveryID)
+		assert.Equal(t, task.AttemptNum, got.AttemptNum)
+	case <-time.After(5 * time.Second):
+		t.Fatal("restart recovery did not queue the delivery again")
+	}
+}
+
+// TestEngine_StopLeavesFireAndForgetAttemptPending is the regression
+// test for a stop that turned an attempt in flight into a terminal
+// failure. Stopping the engine cancels the request; for a target with
+// no retries that was the delivery's only attempt, recorded as failed
+// with "context canceled", and nothing ever sent it again. The
+// delivery must instead stay pending, with no result, for restart
+// recovery to send.
+func TestEngine_StopLeavesFireAndForgetAttemptPending(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+	lc := startEngineViaHook(t, s.Engine)
+
+	task, arrived := seedHeldDelivery(
+		t, s, 0, database.DeliveryStatusPending, 1,
+	)
+
+	s.Engine.Notify([]delivery.Task{task})
+	stopDuringAttempt(t, lc, arrived)
+
+	iAssertStatus(
+		t, s.WebhookDB, task.DeliveryID, database.DeliveryStatusPending,
+	)
+	assert.Empty(t, iResults(t, s.WebhookDB, task.DeliveryID))
+
+	requireRequeued(t, s, s.Engine.ExportDeliveryCh(), task)
+}
+
+// TestEngine_StopLeavesLastRetryAttemptRetrying is the same regression
+// for a target with retries, on its last attempt: the cancelled attempt
+// used to exhaust the retries and end the delivery failed. It must stay
+// retrying with only its earlier attempt recorded, and restart recovery
+// must send that last attempt again.
+func TestEngine_StopLeavesLastRetryAttemptRetrying(t *testing.T) {
+	t.Parallel()
+
+	const maxRetries = 2
+
+	s := newISetup(t)
+	lc := startEngineViaHook(t, s.Engine)
+
+	task, arrived := seedHeldDelivery(
+		t, s, maxRetries, database.DeliveryStatusRetrying, maxRetries,
+	)
+	iSeedFailedResult(t, s.WebhookDB, task.DeliveryID)
+
+	s.Engine.ExportScheduleRetry(task, 0)
+	stopDuringAttempt(t, lc, arrived)
+
+	iAssertStatus(
+		t, s.WebhookDB, task.DeliveryID, database.DeliveryStatusRetrying,
+	)
+	assert.Len(t, iResults(t, s.WebhookDB, task.DeliveryID), 1)
+
+	requireRequeued(t, s, s.Engine.ExportRetryCh(), task)
+}
+
 // deliverToArchive gives the setup's webhook a database target,
 // runs one delivery to it through the running engine, and returns
 // the target's ID and archive file path. The archive writer holds

internal/delivery/target_http.go

@@ -57,7 +57,11 @@ type httpCore struct {
 // recorded and no circuit breaker is consulted. A positive
 // maxRetries gates the attempt on the circuit breaker and
 // schedules a backed-off retry on failure.
+//
+// ctx is the context attempt sends its request with. Stopping the
+// engine cancels it; see interruptedByStop.
 func (c *httpCore) deliver(
+	ctx context.Context,
 	webhookDB *gorm.DB,
 	d *database.Delivery,
 	task *Task,
@@ -66,23 +70,57 @@ func (c *httpCore) deliver(
 	attempt func() attemptResult,
 ) {
 	if maxRetries == 0 {
-		c.fireAndForget(webhookDB, d, attempt())
+		c.fireAndForget(ctx, webhookDB, d, attempt())
 
 		return
 	}
 
 	c.withRetry(
-		webhookDB, d, task, sched, maxRetries, attempt,
+		ctx, webhookDB, d, task, sched, maxRetries, attempt,
 	)
 }
 
+// interruptedByStop reports whether an attempt failed because the
+// engine is stopping, which cancels the context its request was sent
+// with. Such an attempt is not recorded: no DeliveryResult, no circuit
+// breaker outcome, no status change. The delivery stays pending or
+// retrying, and startup recovery sends it again on the next start.
+// The request did go out, so the attempt metrics still count it.
+//
+// A target's own timeout leaves that context alone, so its attempt is
+// still recorded as the failure it is.
+func (c *httpCore) interruptedByStop(
+	ctx context.Context,
+	d *database.Delivery,
+	res attemptResult,
+) bool {
+	if res.success || ctx.Err() == nil {
+		return false
+	}
+
+	c.eng.log.Warn(
+		"delivery attempt interrupted by shutdown; not recorded, "+
+			"it is sent again on the next start",
+		"delivery_id", d.ID,
+		"event_id", d.EventID,
+		"target_id", d.TargetID,
+	)
+
+	return true
+}
+
 func (c *httpCore) fireAndForget(
+	ctx context.Context,
 	webhookDB *gorm.DB,
 	d *database.Delivery,
 	res attemptResult,
 ) {
 	c.eng.observeAttempt(d.Target.Type, res.elapsed())
 
+	if c.interruptedByStop(ctx, d, res) {
+		return
+	}
+
 	err := c.eng.recordResult(
 		webhookDB, d, 1, res.success,
 		res.statusCode, res.respBody, res.errMsg,
@@ -110,6 +148,7 @@ func (c *httpCore) fireAndForget(
 }
 
 func (c *httpCore) withRetry(
+	ctx context.Context,
 	webhookDB *gorm.DB,
 	d *database.Delivery,
 	task *Task,
@@ -133,6 +172,10 @@ func (c *httpCore) withRetry(
 
 	c.eng.observeAttempt(d.Target.Type, res.elapsed())
 
+	if c.interruptedByStop(ctx, d, res) {
+		return
+	}
+
 	err := c.eng.recordResult(
 		webhookDB, d, attemptNum, res.success,
 		res.statusCode, res.respBody, res.errMsg,
@@ -395,7 +438,7 @@ func (t *httpTarget) Deliver(
 	}
 
 	t.deliver(
-		webhookDB, d, task, sched,
+		ctx, webhookDB, d, task, sched,
 		d.Target.MaxRetries, attempt,
 	)
 }

internal/delivery/target_slack.go

@@ -82,7 +82,7 @@ func (t *slackTarget) Deliver(
 	}
 
 	t.deliver(
-		webhookDB, d, task, sched,
+		ctx, webhookDB, d, task, sched,
 		d.Target.MaxRetries, attempt,
 	)
 }