Store a webhook's retention as a plain day count, 0 meaning forever (closes #539)

retention_days loses its column default of 30, and Webhook.RetentionDays
its default tag, so a 0 is stored as 0. The 365000-day stand-in
RetentionForeverDays and the BeforeSave hook that wrote it are gone. The
reaper and the "forever" label read 0 as forever, and the forms accept 0
or 1 to MaxFiniteRetentionDays; a blank field still gives 30 on the new
webhook form and keeps the stored value on the edit form. A negative
value, which the forms refuse, is still read as forever so it cannot put
the reaper's cutoff in the future.

Model: opus-5-5

internal/database/retention_test.go

@@ -85,41 +85,6 @@ func createWebhook(
 		db.Omit(clause.Associations).Create(wh).Error,
 	)
 
-	// Webhook.BeforeSave rewrites a non-positive RetentionDays to the
-	// retain-forever sentinel, and the column's GORM default would
-	// otherwise substitute 30. Force the requested value with a
-	// column-level update so tests can plant legacy rows that predate
-	// the sentinel and still carry a literal 0 or negative value.
-	require.NoError(
-		t,
-		db.Model(wh).
-			Update("retention_days", retentionDays).Error,
-	)
-
-	return wh.ID
-}
-
-// createWebhookNormally inserts a webhook through the ordinary Create
-// path, with no column-level forcing, so Webhook.BeforeSave applies
-// exactly as it does in production. Passing 0 therefore yields a row
-// holding the RetentionForeverDays sentinel.
-func createWebhookNormally(
-	t *testing.T,
-	db *gorm.DB,
-	retentionDays int,
-) string {
-	t.Helper()
-
-	wh := &database.Webhook{
-		UserID:        uuid.New().String(),
-		Name:          testWebhookName,
-		RetentionDays: retentionDays,
-	}
-	require.NoError(
-		t,
-		db.Omit(clause.Associations).Create(wh).Error,
-	)
-
 	return wh.ID
 }
 
@@ -281,19 +246,19 @@ func TestRetentionReaper_ReapsExpiredKeepsRecent(t *testing.T) {
 	assertChainPresent(t, db, recent)
 }
 
-// TestRetentionReaper_SkipsSentinelReapsFiniteInSameSweep covers the
-// end-to-end retain-forever path: a webhook created the normal way with
-// a requested retention of 0 lands on the RetentionForeverDays
-// sentinel, and the reaper leaves its ancient events alone while still
-// reaping a finite-retention webhook in the very same sweep.
-func TestRetentionReaper_SkipsSentinelReapsFiniteInSameSweep(
+// TestRetentionReaper_SkipsZeroReapsFiniteInSameSweep covers the
+// end-to-end retain-forever path: a webhook created with a retention
+// of 0 reads back 0, and the reaper leaves its ancient events alone
+// while still reaping a finite-retention webhook in the very same
+// sweep.
+func TestRetentionReaper_SkipsZeroReapsFiniteInSameSweep(
 	t *testing.T,
 ) {
 	t.Parallel()
 
 	env := setupRetentionTest(t)
 
-	foreverID := createWebhookNormally(t, env.mainDB.DB(), 0)
+	foreverID := createWebhook(t, env.mainDB.DB(), 0)
 
 	var stored database.Webhook
 
@@ -302,14 +267,12 @@ func TestRetentionReaper_SkipsSentinelReapsFiniteInSameSweep(
 		env.mainDB.DB().Where("id = ?", foreverID).
 			First(&stored).Error,
 	)
-	require.Equal(
-		t,
-		database.RetentionForeverDays,
-		stored.RetentionDays,
-		"a requested retention of 0 must persist as the sentinel",
+	require.Zero(
+		t, stored.RetentionDays,
+		"a retention of 0 must be stored as 0",
 	)
 
-	finiteID := createWebhookNormally(t, env.mainDB.DB(), 30)
+	finiteID := createWebhook(t, env.mainDB.DB(), 30)
 
 	foreverDB, err := env.mgr.GetDB(foreverID)
 	require.NoError(t, err)
@@ -335,14 +298,10 @@ func TestRetentionReaper_SkipsSentinelReapsFiniteInSameSweep(
 // negative, so subtracting it moves the cutoff into the far future,
 // where "created_at < cutoff" matches every row: an event created a
 // moment ago, and its delivery and delivery result, were all deleted
-// on the first sweep. 200000 is inside that band and below the
-// retain-forever sentinel, so it is treated as a finite policy and
-// really does reach the arithmetic.
+// on the first sweep. 200000 is inside that band.
 //
-// The row is planted at the column level because such a value can no
-// longer be submitted through the form; the point of the test is that
-// a row from an older version, or a future call site, still cannot
-// trigger the wipe.
+// The form refuses such a value; the point of the test is that a row
+// written by any other path still cannot trigger the wipe.
 func TestRetentionReaper_HugeFiniteRetentionRetainsRecentEvents(
 	t *testing.T,
 ) {
@@ -358,12 +317,6 @@ func TestRetentionReaper_HugeFiniteRetentionRetainsRecentEvents(
 		database.MaxFiniteRetentionDays,
 		"the test value must exceed what the cutoff can represent",
 	)
-	require.Less(
-		t,
-		overflowingRetentionDays,
-		database.RetentionForeverDays,
-		"the test value must not be treated as retain-forever",
-	)
 
 	webhookID := createWebhook(
 		t, env.mainDB.DB(), overflowingRetentionDays,
@@ -379,14 +332,15 @@ func TestRetentionReaper_HugeFiniteRetentionRetainsRecentEvents(
 	assertChainPresent(t, db, fresh)
 }
 
-func TestRetentionReaper_RetainsForeverWhenNonPositive(t *testing.T) {
+// TestRetentionReaper_RetainsForeverWhenNegative checks that a negative
+// retention, which the forms refuse, keeps every event rather than
+// putting the cutoff in the future, where it would match them all.
+func TestRetentionReaper_RetainsForeverWhenNegative(t *testing.T) {
 	t.Parallel()
 
 	env := setupRetentionTest(t)
 
-	// A legacy row written before the sentinel existed still carries a
-	// literal 0; the <= 0 guard must keep honouring it.
-	webhookID := createWebhook(t, env.mainDB.DB(), 0)
+	webhookID := createWebhook(t, env.mainDB.DB(), -1)
 
 	db, err := env.mgr.GetDB(webhookID)
 	require.NoError(t, err)