fix(deploy): SetTTL terminal/permanent guard authoritative in SQL — close TOCTOU (#237)

Design-review follow-up to #228. SetTTL guarded permanent + terminal deploys
via a PRE-READ of the looked-up row, then called SetDeploymentTTL whose UPDATE
only guarded `ttl_policy != 'permanent'`. Two problems the panel surfaced:

  1. TOCTOU: a deploy reaped to expired/deleted/stopped (or made permanent)
     between the handler's status read and the UPDATE would still get
     expires_at re-armed + reminders_sent reset to 0 — re-entering the
     6-email reminder cycle for a deploy the user thinks is gone. A lost race
     also returned 200 "TTL set" for a write that never happened.
  2. The terminal SQL guard was missing entirely (only permanent was guarded).

Fix: make the status-guarded UPDATE the AUTHORITATIVE guard. SetDeploymentTTL
now `WHERE id=$2 AND ttl_policy != 'permanent' AND status NOT IN
('deleted','expired','stopped')` and returns whether a row was actually
updated. The handler drops the stale pre-read; on !applied it re-reads (now
authoritative, not racy) ONLY to pick the precise code — already_permanent vs
invalid_state — so a concurrent change yields an honest 409, never a phantom
200.

Added lifecycleTerminalStatusesSQL (deleted/expired/stopped) as a NAMED const,
deliberately distinct from terminalDeploymentStatusesSQL (deleted/expired):
the former is "reject lifecycle mutations" (stopped is terminal), the latter
is "hide from the user's list" (stopped is still visible). Documented why they
differ — the panel's "unify them" call was wrong; they answer different
questions.

Tests: model SetDeploymentTTL now 100% (added the 0-rows→applied=false sqlmock
case); the existing TestSetTTL_{Permanent,Terminal}Rejected now deterministically
cover the handler's re-read !applied sub-branches (permanent→already_permanent,
terminal→invalid_state). Signature change threaded through 3 callers. Verified
vs real Postgres+Redis; all changed lines covered.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: mastermanas805

internal/handlers/deploy_ttl.go

@@ -143,19 +143,6 @@ func (h *DeployHandler) SetTTL(c *fiber.Ctx) error {
 		return respondError(c, fiber.StatusNotFound, "not_found", "Deployment not found")
 	}
 
-	// Bug-bash #3/#5/#6: a permanent deploy must not be silently flipped back to
-	// an expiring TTL, and a terminal (expired/deleted/stopped) deploy can't have
-	// its lifecycle changed. Guard both before the write (the model adds a
-	// defense-in-depth WHERE clause for the permanent case too).
-	if d.TTLPolicy == models.DeployTTLPolicyPermanent {
-		return respondError(c, fiber.StatusConflict, "already_permanent",
-			"This deployment is permanent — it has no TTL to set. (Downgrading a permanent deploy is support-only.)")
-	}
-	if models.IsDeploymentTerminal(d.Status) {
-		return respondError(c, fiber.StatusConflict, "invalid_state",
-			fmt.Sprintf("Cannot set a TTL on a %s deployment", d.Status))
-	}
-
 	if team.PlanTier == "anonymous" {
 		// B7-P1-7 (see MakePermanent above): emit `claim_required` so an
 		// agent branching on error code routes the user to the free claim
@@ -180,13 +167,33 @@ func (h *DeployHandler) SetTTL(c *fiber.Ctx) error {
 			"")
 	}
 
-	if err := models.SetDeploymentTTL(c.Context(), h.db, d.ID, body.Hours); err != nil {
+	// The status-guarded UPDATE is the AUTHORITATIVE guard (bug-bash #3/#5/#6,
+	// hardened post-review): it only touches a row whose ttl_policy != 'permanent'
+	// AND whose status is non-terminal. Doing the check in SQL — rather than a
+	// pre-read of the looked-up row — removes the check-then-act TOCTOU where a
+	// deploy reaped/made-permanent between the read and the write would still get
+	// a re-armed TTL + reminders_sent=0 reset.
+	applied, err := models.SetDeploymentTTL(c.Context(), h.db, d.ID, body.Hours)
+	if err != nil {
 		slog.Error("deploy.set_ttl.failed",
 			"deploy_id", d.ID, "team_id", team.ID, "error", err,
 			"request_id", middleware.GetRequestID(c))
 		return respondError(c, fiber.StatusServiceUnavailable, "update_failed",
 			"Failed to set TTL")
 	}
+	if !applied {
+		// The UPDATE matched no row → the deploy is permanent or terminal. Re-read
+		// (authoritative, not a stale pre-read) only to choose the precise code:
+		// `already_permanent` for a permanent deploy, else `invalid_state`. A
+		// fetch error here still yields an honest 409 rather than a phantom 200.
+		cur, ferr := models.GetDeploymentByID(c.Context(), h.db, d.ID)
+		if ferr == nil && cur.TTLPolicy == models.DeployTTLPolicyPermanent {
+			return respondError(c, fiber.StatusConflict, "already_permanent",
+				"This deployment is permanent — it has no TTL to set. (Downgrading a permanent deploy is support-only.)")
+		}
+		return respondError(c, fiber.StatusConflict, "invalid_state",
+			"Cannot set a TTL: the deployment is in a terminal state (expired/deleted/stopped) or changed concurrently.")
+	}
 
 	updated, err := models.GetDeploymentByID(c.Context(), h.db, d.ID)
 	if err != nil {

internal/handlers/deploy_ttl_guards_test.go

@@ -92,8 +92,11 @@ func TestSetDeploymentTTL_PermanentGuard(t *testing.T) {
 	_, err := db.Exec(`UPDATE deployments SET ttl_policy='permanent', expires_at=NULL WHERE id=$1`, deployID)
 	require.NoError(t, err)
 
-	// Direct model call must NOT flip the permanent row.
-	require.NoError(t, models.SetDeploymentTTL(context.Background(), db, deployID, 24))
+	// Direct model call must NOT flip the permanent row — the WHERE guard
+	// matches no row, so it reports applied=false (no error).
+	appliedPerm, err := models.SetDeploymentTTL(context.Background(), db, deployID, 24)
+	require.NoError(t, err)
+	require.False(t, appliedPerm, "permanent row must not be updated → applied=false")
 	var policy string
 	var expiresAt sql.NullTime
 	require.NoError(t, db.QueryRow(`SELECT ttl_policy, expires_at FROM deployments WHERE id=$1`, deployID).Scan(&policy, &expiresAt))

internal/models/coverage_deployment_test.go

@@ -201,10 +201,19 @@ func TestDeploymentSimpleUpdaters_Branches(t *testing.T) {
 	// SetDeploymentTTL
 	db8, mock8 := newMock(t)
 	mock8.ExpectExec(`SET expires_at = \$1,\s+ttl_policy = 'custom'`).WillReturnResult(sqlmock.NewResult(0, 1))
-	require.NoError(t, SetDeploymentTTL(ctx, db8, id, 48))
+	ok8, err8 := SetDeploymentTTL(ctx, db8, id, 48)
+	require.NoError(t, err8)
+	require.True(t, ok8, "1 row affected → applied")
 	db8b, mock8b := newMock(t)
 	mock8b.ExpectExec(`ttl_policy = 'custom'`).WillReturnError(errors.New("boom"))
-	require.ErrorContains(t, SetDeploymentTTL(ctx, db8b, id, 48), "boom")
+	_, err8b := SetDeploymentTTL(ctx, db8b, id, 48)
+	require.ErrorContains(t, err8b, "boom")
+	// 0 rows affected (permanent/terminal guard matched nothing) → applied=false.
+	db8c, mock8c := newMock(t)
+	mock8c.ExpectExec(`ttl_policy = 'custom'`).WillReturnResult(sqlmock.NewResult(0, 0))
+	ok8c, err8c := SetDeploymentTTL(ctx, db8c, id, 48)
+	require.NoError(t, err8c)
+	require.False(t, ok8c, "0 rows affected → not applied")
 
 	// MarkDeploymentExpired
 	db9, mock9 := newMock(t)

internal/models/deployment.go

@@ -738,25 +738,34 @@ func ElevateDeploymentTiersByTeam(ctx context.Context, db *sql.DB, teamID uuid.U
 // (1..8760) BEFORE invoking this — the model trusts its input. Resets
 // reminders_sent so a freshly-extended deploy gets the full 6-email
 // warning cycle again instead of skipping reminders that fired earlier.
-func SetDeploymentTTL(ctx context.Context, db *sql.DB, id uuid.UUID, hours int) error {
+func SetDeploymentTTL(ctx context.Context, db *sql.DB, id uuid.UUID, hours int) (bool, error) {
 	expiresAt := time.Now().UTC().Add(time.Duration(hours) * time.Hour)
-	// `ttl_policy != 'permanent'` is defense-in-depth (the handler already
-	// rejects a permanent deploy with 409): if a concurrent MakePermanent races
-	// this UPDATE, the WHERE clause prevents silently un-permanenting the deploy
-	// — the row simply isn't touched (bug-bash #6).
-	_, err := db.ExecContext(ctx, `
+	// The WHERE clause is the AUTHORITATIVE guard, closing the handler's
+	// check-then-act TOCTOU (bug-bash #6, hardened post-review):
+	//   - `ttl_policy != 'permanent'` — a concurrent MakePermanent must not be
+	//     silently un-permanented.
+	//   - `status NOT IN <lifecycleTerminalStatusesSQL>` — a deploy reaped to
+	//     expired/deleted/stopped between the handler's status read and this
+	//     UPDATE must NOT get a re-armed expires_at + reminders_sent=0 reset
+	//     (which would re-enter the reminder-email cycle for a gone deploy).
+	// Returns whether a row was actually updated so the handler can surface a
+	// 409 when the deploy changed underneath it rather than reporting a phantom
+	// success for a write that never happened.
+	res, err := db.ExecContext(ctx, `
 		UPDATE deployments
 		SET expires_at = $1,
 		    ttl_policy = 'custom',
 		    reminders_sent = 0,
 		    last_reminder_at = NULL,
 		    updated_at = now()
 		WHERE id = $2 AND ttl_policy != 'permanent'
+		  AND status NOT IN `+lifecycleTerminalStatusesSQL+`
 	`, expiresAt, id)
 	if err != nil {
-		return fmt.Errorf("models.SetDeploymentTTL: %w", err)
+		return false, fmt.Errorf("models.SetDeploymentTTL: %w", err)
 	}
-	return nil
+	n, _ := res.RowsAffected()
+	return n > 0, nil
 }
 
 // GetDeploymentsExpiringSoon returns deployments whose expires_at falls
@@ -955,6 +964,17 @@ const activeDeploymentStatusesSQL = `('building', 'deploying', 'healthy')`
 // "user-visible deployments" row set — see deploymentVisibleClause.
 const terminalDeploymentStatusesSQL = `('deleted', 'expired')`
 
+// lifecycleTerminalStatusesSQL is the IN-list for "this deploy has reached an
+// end state — reject lifecycle MUTATIONS on it." It deliberately DIFFERS from
+// terminalDeploymentStatusesSQL: that one answers "hide from the user's list /
+// usage count" (a 'stopped' deploy is still VISIBLE — paused, not gone — so
+// it's excluded there). This one answers "may we re-arm a TTL on it", where
+// 'stopped' IS terminal — matching IsDeploymentTerminal. The two lists encode
+// different questions; keeping them separate is intentional, not drift. Used
+// by SetDeploymentTTL's UPDATE guard so a deploy reaped between the handler's
+// status read and the UPDATE cannot get a re-armed TTL + reminder-cycle reset.
+const lifecycleTerminalStatusesSQL = `('deleted', 'expired', 'stopped')`
+
 // deploymentVisibleClause is the shared WHERE predicate for "deployments the
 // user sees" — i.e. every non-terminal row. GET /api/v1/deployments (the list)
 // and GET /api/v1/billing/usage's deployment count MUST use this same clause

internal/models/deployment_ttl_test.go

@@ -162,7 +162,9 @@ func TestSetDeploymentTTL_SetsCustomExpiryAndResetsReminders(t *testing.T) {
 	`, d.ID)
 	require.NoError(t, err)
 
-	require.NoError(t, models.SetDeploymentTTL(ctx, db, d.ID, 72))
+	appliedTTL, err := models.SetDeploymentTTL(ctx, db, d.ID, 72)
+	require.NoError(t, err)
+	require.True(t, appliedTTL, "healthy deploy → TTL applied")
 
 	refreshed, err := models.GetDeploymentByID(ctx, db, d.ID)
 	require.NoError(t, err)