fix(db): contract the canceled rename: normalize, narrow the status CHECKs, write only canceled_by

internal/api/handler_purchases_cancel_actor_integration_test.go

@@ -39,7 +39,7 @@ func TestDeletePlannedPurchase_RecordsCancelActor(t *testing.T) {
 	require.Equal(t, config.StatusCanceled, row.Status)
 	require.NotNil(t, row.CancelledBy, "cancel actor must be readable after a UI cancel")
 	require.Equal(t, "creator@example.com", *row.CancelledBy)
-	requireRawCancelColumns(ctx, t, f, execID, "creator@example.com")
+	requireRawCanceledBy(ctx, t, f, execID, "creator@example.com")
 
 	// The record TransitionExecutionStatus returns must use the same projection.
 	directID := uuid.NewString()
@@ -55,7 +55,7 @@ func TestDeletePlannedPurchase_RecordsCancelActor(t *testing.T) {
 
 // TestTransitionExecutionStatus_NonCancelLeavesCancelColumnsNull pins the
 // cancel-only guard: a non-cancel transition with an actor stamps
-// transitioned_by but must not write either cancel column.
+// transitioned_by but must not write canceled_by.
 func TestTransitionExecutionStatus_NonCancelLeavesCancelColumnsNull(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second)
 	t.Cleanup(cancel)
@@ -72,26 +72,19 @@ func TestTransitionExecutionStatus_NonCancelLeavesCancelColumnsNull(t *testing.T
 	_, err := f.store.TransitionExecutionStatus(ctx, execID, []string{"pending"}, config.StatusFailed, &creator)
 	require.NoError(t, err)
 
-	var canceledBy, cancelledBy *string
+	var canceledBy *string
 	require.NoError(t, f.pool.QueryRow(ctx,
-		"SELECT canceled_by, "+legacyCancelColumn+" FROM purchase_executions WHERE execution_id=$1", execID,
-	).Scan(&canceledBy, &cancelledBy))
+		"SELECT canceled_by FROM purchase_executions WHERE execution_id=$1", execID,
+	).Scan(&canceledBy))
 	require.Nil(t, canceledBy, "canceled_by must stay NULL on a non-cancel transition")
-	require.Nil(t, cancelledBy, legacyCancelColumn+" must stay NULL on a non-cancel transition")
 }
 
-func requireRawCancelColumns(ctx context.Context, t *testing.T, f *createConcurrencyFixture, execID, want string) {
+func requireRawCanceledBy(ctx context.Context, t *testing.T, f *createConcurrencyFixture, execID, want string) {
 	t.Helper()
-	var canceledBy, cancelledBy *string
+	var canceledBy *string
 	require.NoError(t, f.pool.QueryRow(ctx,
-		"SELECT canceled_by, "+legacyCancelColumn+" FROM purchase_executions WHERE execution_id=$1", execID,
-	).Scan(&canceledBy, &cancelledBy))
+		"SELECT canceled_by FROM purchase_executions WHERE execution_id=$1", execID,
+	).Scan(&canceledBy))
 	require.NotNil(t, canceledBy, "canceled_by column")
-	require.NotNil(t, cancelledBy, legacyCancelColumn+" column")
 	require.Equal(t, want, *canceledBy)
-	require.Equal(t, want, *cancelledBy)
 }
-
-// legacyCancelColumn is the pre-migration-000089 British spelling, split so the
-// misspell linter does not flag the column name.
-const legacyCancelColumn = "cancel" + "led_by"

internal/api/handler_purchases_legacy_cancel_integration_test.go

@@ -0,0 +1,77 @@
+//go:build integration
+
+package api
+
+import (
+	"context"
+	"os"
+	"strconv"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers"
+	"github.com/stretchr/testify/require"
+)
+
+// versionBeforeCanceledContract is the schema a deployment is at when the
+// build under test starts: the highest migration below the canceled contract
+// (the file ending _contract_canceled_spelling.up.sql), which narrows the
+// status CHECKs. The cutoff is read from that file, so a renumber needs no edit
+// here. The new build must read and write correctly there, because the
+// migration and the release roll out in either order.
+func versionBeforeCanceledContract(t *testing.T, dir string) uint {
+	t.Helper()
+	entries, err := os.ReadDir(dir)
+	require.NoError(t, err)
+	prefix := func(name string) (uint, bool) {
+		v, err := strconv.ParseUint(strings.SplitN(name, "_", 2)[0], 10, 32)
+		return uint(v), err == nil
+	}
+	var contract uint
+	for _, e := range entries {
+		if v, ok := prefix(e.Name()); ok && strings.HasSuffix(e.Name(), "_contract_canceled_spelling.up.sql") {
+			contract = v
+		}
+	}
+	require.NotZero(t, contract, "no *_contract_canceled_spelling.up.sql migration in %s", dir)
+	var best uint
+	for _, e := range entries {
+		if v, ok := prefix(e.Name()); ok && v < contract && v > best {
+			best = v
+		}
+	}
+	require.NotZero(t, best, "no migration below %d", contract)
+	return best
+}
+
+// TestCancelOrRecoverExecution_LegacyCanceledRowBeforeContractMigration runs
+// the cancel-race recovery on the schema before 000157 against a row an older
+// build canceled (legacy status spelling, actor only in the legacy column). The
+// build and the migration roll out in either order, so the retry must see the
+// race as already won, return the canonical status and keep the actor.
+func TestCancelOrRecoverExecution_LegacyCanceledRowBeforeContractMigration(t *testing.T) {
+	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute)
+	t.Cleanup(cancel)
+	container := testhelpers.RequireEmptyDatabase(ctx, t)
+	pool := container.DB.Pool()
+	path := getMigrationsPath()
+	before := versionBeforeCanceledContract(t, path)
+	require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, before))
+	store := config.NewPostgresStore(container.DB)
+
+	var execID string
+	require.NoError(t, pool.QueryRow(ctx, `
+		INSERT INTO purchase_executions (status, scheduled_date, `+"cancel"+`led_by)
+		VALUES ($1, NOW(), 'legacy@example.com') RETURNING execution_id::text`,
+		config.LegacyStatusCanceled).Scan(&execID))
+
+	h := &Handler{config: store}
+	got, err := h.cancelOrRecoverExecution(ctx, execID, nil)
+	require.NoError(t, err, "a legacy canceled row is an already-won cancel race, not a 409")
+	require.Equal(t, config.StatusCanceled, got.Status, "the idempotent path returns the canonical spelling")
+	require.NotNil(t, got.CancelledBy)
+	require.Equal(t, "legacy@example.com", *got.CancelledBy)
+}

internal/api/handler_purchases_submit_dedupe_integration_test.go

@@ -119,7 +119,7 @@ func TestPersistExecution_RetryAfterRowLeftPending(t *testing.T) {
 	}{
 		{"pending", true}, {"notified", true}, {"approved", true}, {"running", true},
 		{"completed", true}, {"partially_completed", true},
-		{config.StatusFailed, false}, {config.StatusCanceled, false}, {config.LegacyStatusCanceled, false}, {config.StatusExpired, false},
+		{config.StatusFailed, false}, {config.StatusCanceled, false}, {config.StatusExpired, false},
 	} {
 		t.Run(tc.status, func(t *testing.T) {
 			recs, key := submitFixture(t)

internal/config/interfaces.go

@@ -289,11 +289,10 @@ type StoreInterface interface {
 	// (and writes nothing) when the row has left those statuses, so a cancel or
 	// approve that lands first is never overwritten.
 	RescheduleAwaitingExecution(ctx context.Context, executionID string, scheduledDate time.Time) (bool, error)
-	// SetCancelledBy stamps canceled_by and the legacy cancelled_by column on
-	// an execution without overwriting any other columns. Used after
-	// TransitionExecutionStatus to fold the actor attribution into the same
-	// logical write without the full-row SavePurchaseExecution clobber risk
-	// (Finding #5 / PR #889).
+	// SetCancelledBy stamps canceled_by on an execution without overwriting
+	// any other columns. Used after TransitionExecutionStatus to fold the actor
+	// attribution into the same logical write without the full-row
+	// SavePurchaseExecution clobber risk (Finding #5 / PR #889).
 	SetCancelledBy(ctx context.Context, executionID string, cancelledBy string) error
 	// LinkRetryExecutionAtomic stamps retry_execution_id on a failed
 	// execution row without a full-row overwrite, conditional on

internal/config/store_postgres.go

@@ -1169,12 +1169,16 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx,
 		capacityPercent = 100
 	}
 
+	// canceled_by keeps an actor already on the row: a save from a copy read
+	// before a concurrent cancel must not erase that cancel's attribution. Reads
+	// take COALESCE(canceled_by, cancelled_by), so a legacy actor on cancelled_by
+	// is carried into canceled_by here. This save no longer writes cancelled_by.
 	query := `
 		INSERT INTO purchase_executions (
 			plan_id, execution_id, status, step_number, scheduled_date,
 			notification_sent, approval_token_hash, recommendations,
 			total_upfront_cost, estimated_savings, completed_at, error, expires_at,
-			cloud_account_id, source, approved_by, cancelled_by, capacity_percent,
+			cloud_account_id, source, approved_by, canceled_by, capacity_percent,
 			created_by_user_id, retry_execution_id, retry_attempt_n,
 			approval_token_expires_at,
 			executed_by_user_id, executed_at, pre_approval_skip_reason,
@@ -1193,7 +1197,7 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx,
 			cloud_account_id = $14,
 			source = $15,
 			approved_by = $16,
-			cancelled_by = $17,
+			canceled_by = COALESCE(purchase_executions.canceled_by, $17),
 			capacity_percent = $18,
 			approval_token_expires_at = $22,
 			executed_by_user_id = $23,
@@ -1334,8 +1338,8 @@ func (s *PostgresStore) RotatePendingApprovalToken(ctx context.Context, executio
 // actor is the UUID of the user performing the transition (nil for system-initiated paths);
 // it is stamped onto transitioned_by and transitioned_at is always set to NOW().
 // A transition to StatusCanceled also stamps the actor's email onto canceled_by
-// and the legacy cancelled_by (issue #119), matching what the other cancel paths
-// record and what History reads.
+// (issue #119), matching what the other cancel paths record and what History
+// reads.
 func (s *PostgresStore) TransitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string, actor *string) (*PurchaseExecution, error) {
 	return s.transitionExecutionStatus(ctx, s.db, executionID, fromStatuses, toStatus, actor, nil, nil)
 }
@@ -1414,8 +1418,7 @@ func (s *PostgresStore) transitionExecutionStatus(ctx context.Context, q executi
 		UPDATE purchase_executions
 		SET status = $2, updated_at = NOW(),
 		    transitioned_by = $4, transitioned_at = NOW(),
-		    canceled_by = COALESCE((SELECT email FROM users WHERE id = $5), canceled_by),
-		    cancelled_by = COALESCE((SELECT email FROM users WHERE id = $5), cancelled_by)
+		    canceled_by = COALESCE((SELECT email FROM users WHERE id = $5), canceled_by)
 		WHERE execution_id = $1 AND status = ANY($3)
 		  AND ($6::boolean IS NULL OR (recommendations IN ('[]'::jsonb, 'null'::jsonb)) = $6)
 		  AND ($7::text IS NULL OR (approval_token_hash = $7
@@ -1472,16 +1475,14 @@ func transitionRejection(ctx context.Context, q executionQuerier, executionID st
 	return fmt.Errorf("%w: execution %s cannot transition from %q to %q", ErrExecutionNotInExpectedStatus, executionID, existing.Status, toStatus)
 }
 
-// SetCancelledBy stamps both the canceled_by and legacy cancelled_by columns
-// for a single execution without touching any other column. This avoids the
-// full-row overwrite that a SavePurchaseExecution follow-up would perform,
-// eliminating the lost-update window between TransitionExecutionStatus and
-// the attribution write (Finding #5). Writing both columns keeps this path
-// symmetric with CancelExecutionAtomic/CancelScheduledExecutionAtomic (which
-// write canceled_by only) so revoke-attribution keeps working unchanged if a
-// future contract migration (#1278) drops the legacy column.
+// SetCancelledBy stamps canceled_by for a single execution without touching
+// any other column. This avoids the full-row overwrite that a
+// SavePurchaseExecution follow-up would perform, eliminating the lost-update
+// window between TransitionExecutionStatus and the attribution write
+// (Finding #5). It no longer writes the legacy cancelled_by column; reads take
+// COALESCE(canceled_by, cancelled_by).
 func (s *PostgresStore) SetCancelledBy(ctx context.Context, executionID, cancelledBy string) error {
-	q := `UPDATE purchase_executions SET canceled_by = $2, cancelled_by = $2, updated_at = NOW()
+	q := `UPDATE purchase_executions SET canceled_by = $2, updated_at = NOW()
 	       WHERE execution_id = $1`
 	_, err := s.db.Exec(ctx, q, executionID, cancelledBy)
 	return err

internal/config/store_postgres_cancel_actor_pgxmock_test.go

@@ -0,0 +1,147 @@
+package config
+
+import (
+	"context"
+	"errors"
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/pashagolub/pgxmock/v5"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// legacyCancelActorColumn is the pre-migration-000089 column, split so the
+// US-locale misspell linter does not flag it; it is the value under test.
+const legacyCancelActorColumn = "cancel" + "led_by"
+
+const coalescedActorProjection = "COALESCE(canceled_by, " + legacyCancelActorColumn + ") AS " + legacyCancelActorColumn
+
+var executionProjectionCols = []string{
+	"plan_id", "execution_id", "status", "step_number", "scheduled_date",
+	"notification_sent", "approval_token", "recommendations",
+	"total_upfront_cost", "estimated_savings", "completed_at", "error", "expires_at",
+	"cloud_account_id", "source", "approved_by", legacyCancelActorColumn, "capacity_percent",
+	"created_by_user_id", "retry_execution_id", "retry_attempt_n",
+	"approval_token_expires_at",
+	"executed_by_user_id", "executed_at", "pre_approval_skip_reason",
+	"idempotency_key", "scheduled_execution_at", "no_submission", "created_by_system",
+}
+
+// TestPGXMock_ExecutionProjectionsReadCoalescedActor pins the read half of the
+// 000089 contract while old builds can still write the legacy column: every
+// statement that projects a purchase_executions row must read
+// COALESCE(canceled_by, <legacy column>), so an actor stored in either column
+// scans back. A bare canceled_by projection loses the attribution of legacy
+// rows (written to the legacy column only) until the follow-up drain runs.
+func TestPGXMock_ExecutionProjectionsReadCoalescedActor(t *testing.T) {
+	var mu sync.Mutex
+	var projections, bare []string
+	mock, err := pgxmock.NewPool(pgxmock.QueryMatcherOption(pgxmock.QueryMatcherFunc(func(_, actual string) error {
+		if !strings.Contains(actual, "capacity_percent") {
+			return nil
+		}
+		mu.Lock()
+		defer mu.Unlock()
+		projections = append(projections, actual)
+		if !strings.Contains(actual, coalescedActorProjection) {
+			bare = append(bare, actual)
+		}
+		return nil
+	})))
+	require.NoError(t, err)
+	mock.MatchExpectationsInOrder(false)
+	store := storeWith(mock)
+	ctx := context.Background()
+	empty := func() *pgxmock.Rows { return pgxmock.NewRows(executionProjectionCols) }
+	// Two rows of expectations per call: a zero-row transition is followed by
+	// a probe SELECT.
+	expect := func() {
+		mock.ExpectQuery(".").WillReturnRows(empty())
+		mock.ExpectQuery(".").WillReturnRows(empty()).Maybe()
+	}
+
+	expect()
+	_, _ = store.GetExecutionsByStatusesFiltered(ctx, ExecutionsByStatusesQuery{Statuses: []string{"pending"}})
+	expect()
+	_, _ = store.GetPlannedExecutions(ctx, []string{"scheduled"}, 10)
+	expect()
+	_, _ = store.GetStaleApprovedExecutions(ctx, time.Hour)
+	expect()
+	_, _ = store.ListStuckExecutions(ctx, []string{"approved"}, time.Hour)
+	expect()
+	_, _ = store.GetPendingExecutions(ctx)
+	expect()
+	_, _ = store.GetExecutionByID(ctx, "exec-1")
+	expect()
+	_, _ = store.GetExecutionByPlanAndDate(ctx, "plan-1", time.Now())
+	expect()
+	_, _ = store.GetScheduledExecutionsDue(ctx)
+	actor := "11111111-1111-1111-1111-111111111111"
+	expect()
+	_, _ = store.TransitionExecutionStatus(ctx, "exec-1", []string{"pending"}, StatusCanceled, &actor)
+	mock.ExpectBegin()
+	mock.ExpectExec(".").WillReturnResult(pgxmock.NewResult("SELECT", 1))
+	expect()
+	tx, err := mock.Begin(ctx)
+	require.NoError(t, err)
+	_, _ = store.ListRecentSubmitsTx(ctx, tx, "creator", time.Now())
+
+	mu.Lock()
+	defer mu.Unlock()
+	assert.GreaterOrEqual(t, len(projections), 10, "every purchase_executions projection must be exercised")
+	assert.Empty(t, bare, "projections that do not read COALESCE(canceled_by, legacy actor)")
+}
+
+// TestPGXMock_ExecutionCancelActorWritesOnlyCanceledBy pins the write half:
+// the save, the cancel transition and SetCancelledBy write canceled_by alone
+// and never the legacy column (RETURNING projections still read it). The
+// matcher rejects any INSERT or UPDATE statement whose write part names the legacy
+// column, so each call fails against the dual-write queries.
+//
+// The save keeps an actor already on the row (COALESCE(existing, $17)): a save
+// from a copy read before a concurrent cancel must not erase that actor, and a
+// legacy-only actor read through COALESCE is carried into canceled_by.
+func TestPGXMock_ExecutionCancelActorWritesOnlyCanceledBy(t *testing.T) {
+	var dualWrites []string
+	mock, err := pgxmock.NewPool(pgxmock.QueryMatcherOption(pgxmock.QueryMatcherFunc(func(expected, actual string) error {
+		write, _, _ := strings.Cut(actual, "RETURNING")
+		verb := strings.ToUpper(strings.Fields(write + " x")[0])
+		if (verb == "INSERT" || verb == "UPDATE") && strings.Contains(write, legacyCancelActorColumn) {
+			dualWrites = append(dualWrites, actual)
+			return errors.New("write still names the legacy cancel actor column: " + actual)
+		}
+		return pgxmock.QueryMatcherRegexp.Match(expected, actual)
+	})))
+	require.NoError(t, err)
+	store := storeWith(mock)
+	ctx := context.Background()
+
+	actor := "11111111-1111-1111-1111-111111111111"
+	mock.ExpectQuery(`(?s)SET status = \$2.*canceled_by = COALESCE\(\(SELECT email FROM users WHERE id = \$5\), canceled_by\)\s+WHERE`).
+		WithArgs(anyArgsCfg(7)...).
+		WillReturnRows(pgxmock.NewRows(executionProjectionCols))
+	// The empty result row makes the store probe the row afterwards; that
+	// probe and its error are not under test.
+	mock.ExpectQuery(`FROM purchase_executions`).WillReturnRows(pgxmock.NewRows(executionProjectionCols)).Maybe()
+	_, _ = store.TransitionExecutionStatus(ctx, "exec-1", []string{"pending"}, StatusCanceled, &actor)
+	require.Empty(t, dualWrites, "the cancel transition must write canceled_by only")
+
+	mock.ExpectExec(`SET canceled_by = \$2, updated_at = NOW\(\)`).
+		WithArgs("exec-1", "revoker@example.com").
+		WillReturnResult(pgxmock.NewResult("UPDATE", 1))
+	require.NoError(t, store.SetCancelledBy(ctx, "exec-1", "revoker@example.com"))
+
+	mock.ExpectBegin()
+	mock.ExpectExec(`(?s)approved_by, canceled_by, capacity_percent.*canceled_by = COALESCE\(purchase_executions\.canceled_by, \$17\)`).
+		WithArgs(anyArgsCfg(30)...).
+		WillReturnResult(pgxmock.NewResult("INSERT", 1))
+	mock.ExpectCommit()
+	require.NoError(t, store.SavePurchaseExecution(ctx, &PurchaseExecution{
+		ExecutionID: "exec-1", PlanID: "plan-1", Status: StatusCanceled, ScheduledDate: time.Now(),
+	}))
+
+	assert.NoError(t, mock.ExpectationsWereMet())
+}

internal/config/store_postgres_db_test.go

@@ -1288,17 +1288,6 @@ func TestPostgresStoreDB_CleanupOldExecutions_RetainsCanceledInsideHealthWindow(
 			shouldSurvive: true,
 			why:           "expires_at branch must not purge what the canceled branch retains",
 		},
-		{
-			// Deliberately the LegacyStatusCanceled constant, i.e. the
-			// pre-migration-000089 double-l DB value. The label avoids
-			// spelling it out only to keep the misspell linter quiet.
-			name:          "legacy-spelling-canceled-today-old-scheduled-date",
-			status:        LegacyStatusCanceled,
-			scheduledDays: -40,
-			updatedDays:   0,
-			shouldSurvive: true,
-			why:           "the legacy spelling is retained on updated_at too",
-		},
 		{
 			// The `failed` counterpart of the canceled case above, and the
 			// reason branch 3's exclusion is sourced from