From 185a2165ccea058955986dceac79040a22554ce1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 21:11:16 +0100 Subject: [PATCH 1/5] feat(config): store purchase attempts for the Redshift journal Add the purchase_attempts table and the PostgresStore RecordAttempt and PriorAttempts methods that implement common.AttemptJournal. The upsert encodes the transition table; offering_id is write-once. Part of LeanerCloud/cloud-commitments-go#107 Co-Authored-By: Claude Sonnet 5.5 --- internal/config/store_postgres_attempts.go | 86 +++++++++++ ...tore_postgres_attempts_integration_test.go | 146 ++++++++++++++++++ .../000158_purchase_attempts.down.sql | 4 + .../000158_purchase_attempts.up.sql | 30 ++++ .../000158_purchase_attempts_test.go | 50 ++++++ 5 files changed, 316 insertions(+) create mode 100644 internal/config/store_postgres_attempts.go create mode 100644 internal/config/store_postgres_attempts_integration_test.go create mode 100644 internal/database/postgres/migrations/000158_purchase_attempts.down.sql create mode 100644 internal/database/postgres/migrations/000158_purchase_attempts.up.sql create mode 100644 internal/database/postgres/migrations/000158_purchase_attempts_test.go diff --git a/internal/config/store_postgres_attempts.go b/internal/config/store_postgres_attempts.go new file mode 100644 index 000000000..dee617411 --- /dev/null +++ b/internal/config/store_postgres_attempts.go @@ -0,0 +1,86 @@ +package config + +import ( + "context" + "fmt" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/jackc/pgx/v5" +) + +// PostgresStore is the platform's common.AttemptJournal (cloud-commitments-go#107). +var _ common.AttemptJournal = (*PostgresStore)(nil) + +// recordAttemptSQL is one autocommit upsert. The WHERE clause is the +// transition table and mirrors validTransition in cloud-commitments-go's +// providers/aws/services/redshift journal test file (a cross-language mirror; +// change both together): +// +// none -> sending | purchased (the INSERT path; the SELECT's +// WHERE refuses none -> outcome_unknown) +// sending -> purchased | outcome_unknown +// outcome_unknown -> purchased +// purchased -> purchased, same reservation_id only +// +// Anything else (including identical sending or outcome_unknown repeats) +// updates no row and RecordAttempt reports an error. offering_id is +// write-once: the first non-NULL value survives every later write. +const recordAttemptSQL = ` +INSERT INTO purchase_attempts (idempotency_token, execution_id, state, reservation_id, offering_id) +SELECT $1::text, $2::text, $3::text, NULLIF($4::text, ''), NULLIF($5::text, '') +WHERE $3::text <> 'outcome_unknown' + OR EXISTS (SELECT 1 FROM purchase_attempts p WHERE p.idempotency_token = $1::text AND p.execution_id = $2::text) +ON CONFLICT (idempotency_token, execution_id) DO UPDATE SET + state = EXCLUDED.state, + reservation_id = EXCLUDED.reservation_id, + offering_id = COALESCE(purchase_attempts.offering_id, EXCLUDED.offering_id), + updated_at = now() +WHERE (purchase_attempts.state = 'sending' AND EXCLUDED.state IN ('purchased', 'outcome_unknown')) + OR (purchase_attempts.state = 'outcome_unknown' AND EXCLUDED.state = 'purchased') + OR (purchase_attempts.state = 'purchased' AND EXCLUDED.state = 'purchased' + AND purchase_attempts.reservation_id = EXCLUDED.reservation_id)` + +// RecordAttempt durably records one purchase attempt. It validates the record, +// then upserts in its own autocommit statement. It must not be called inside a +// caller's transaction: the record has to be committed before it returns. +func (s *PostgresStore) RecordAttempt(ctx context.Context, rec common.AttemptRecord) error { + if err := rec.Validate(); err != nil { + return err + } + tag, err := s.db.Exec(ctx, recordAttemptSQL, + rec.IdempotencyToken, rec.ExecutionID, string(rec.State), rec.ReservationID, rec.OfferingID) + if err != nil { + return fmt.Errorf("record purchase attempt %s: %w", common.MaskToken(rec.IdempotencyToken), err) + } + if tag.RowsAffected() == 0 { + return fmt.Errorf("record purchase attempt %s: transition to %q refused by the existing row", + common.MaskToken(rec.IdempotencyToken), rec.State) + } + return nil +} + +// PriorAttempts returns every attempt of the token across executions. An empty +// result is not an error. Like RecordAttempt it runs on its own connection. +func (s *PostgresStore) PriorAttempts(ctx context.Context, token string) ([]common.AttemptRecord, error) { + rows, err := s.db.Query(ctx, ` + SELECT idempotency_token, execution_id, state, COALESCE(reservation_id, ''), COALESCE(offering_id, '') + FROM purchase_attempts + WHERE idempotency_token = $1 + ORDER BY created_at, execution_id`, token) + if err != nil { + return nil, fmt.Errorf("list purchase attempts %s: %w", common.MaskToken(token), err) + } + recs, err := pgx.CollectRows(rows, func(row pgx.CollectableRow) (common.AttemptRecord, error) { + var r common.AttemptRecord + var state string + if scanErr := row.Scan(&r.IdempotencyToken, &r.ExecutionID, &state, &r.ReservationID, &r.OfferingID); scanErr != nil { + return r, scanErr + } + r.State = common.AttemptState(state) + return r, nil + }) + if err != nil { + return nil, fmt.Errorf("scan purchase attempts %s: %w", common.MaskToken(token), err) + } + return recs, nil +} diff --git a/internal/config/store_postgres_attempts_integration_test.go b/internal/config/store_postgres_attempts_integration_test.go new file mode 100644 index 000000000..84c559d43 --- /dev/null +++ b/internal/config/store_postgres_attempts_integration_test.go @@ -0,0 +1,146 @@ +//go:build integration +// +build integration + +package config + +import ( + "context" + "fmt" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/jackc/pgx/v5/pgconn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const attemptToken = "tok-0123456789abcdef" + +func attemptRec(exec string, state common.AttemptState, id, offering string) common.AttemptRecord { + return common.AttemptRecord{IdempotencyToken: attemptToken, ExecutionID: exec, State: state, ReservationID: id, OfferingID: offering} +} + +// TestRecordAttempt_TransitionTable pins the 16 cases (4 sources x 4 targets) +// of go's validTransition: sources none, sending, outcome_unknown, purchased +// (rn-1); targets sending, outcome_unknown, purchased rn-1, purchased rn-2. +func TestRecordAttempt_TransitionTable(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + + sources := map[string][]common.AttemptRecord{ + "none": nil, + "sending": {attemptRec("", common.AttemptSending, "", "")}, + "outcome_unknown": {attemptRec("", common.AttemptSending, "", ""), attemptRec("", common.AttemptOutcomeUnknown, "", "")}, + "purchased": {attemptRec("", common.AttemptPurchased, "rn-1", "")}, + } + targets := []struct { + name string + rec func(exec string) common.AttemptRecord + // allowed is keyed by source name. + allowed map[string]bool + }{ + {"sending", func(e string) common.AttemptRecord { return attemptRec(e, common.AttemptSending, "", "") }, + map[string]bool{"none": true}}, + {"outcome_unknown", func(e string) common.AttemptRecord { return attemptRec(e, common.AttemptOutcomeUnknown, "", "") }, + map[string]bool{"sending": true}}, + {"purchased_same_id", func(e string) common.AttemptRecord { return attemptRec(e, common.AttemptPurchased, "rn-1", "") }, + map[string]bool{"none": true, "sending": true, "outcome_unknown": true, "purchased": true}}, + {"purchased_other_id", func(e string) common.AttemptRecord { return attemptRec(e, common.AttemptPurchased, "rn-2", "") }, + map[string]bool{"none": true, "sending": true, "outcome_unknown": true}}, + } + + n := 0 + for srcName, setup := range sources { + for _, tgt := range targets { + n++ + exec := fmt.Sprintf("exec-%d", n) + t.Run(srcName+"_to_"+tgt.name, func(t *testing.T) { + for _, s := range setup { + s.ExecutionID = exec + require.NoError(t, store.RecordAttempt(ctx, s)) + } + err := store.RecordAttempt(ctx, tgt.rec(exec)) + if tgt.allowed[srcName] { + require.NoError(t, err) + } else { + require.Error(t, err) + } + }) + } + } + assert.Equal(t, 16, n) +} + +// execCounter counts statements sent to the database, so a test can tell a +// record rejected by Validate from one rejected by a table CHECK. +type execCounter struct { + dbConn + execs int +} + +func (c *execCounter) Exec(ctx context.Context, sql string, args ...any) (pgconn.CommandTag, error) { + c.execs++ + return c.dbConn.Exec(ctx, sql, args...) +} + +func TestRecordAttempt_InvalidRecordRejectedBeforeUpsert(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + counter := &execCounter{dbConn: store.db} + store.db = counter + + require.Error(t, store.RecordAttempt(ctx, attemptRec("exec-1", common.AttemptPurchased, "", "")), "purchased without an ID") + require.Error(t, store.RecordAttempt(ctx, attemptRec("exec-1", common.AttemptSending, "rn-1", "")), "ID on a non-purchased record") + require.Error(t, store.RecordAttempt(ctx, attemptRec("exec-1", common.AttemptState("bogus"), "", "")), "unknown state") + require.Error(t, store.RecordAttempt(ctx, attemptRec("", common.AttemptSending, "", "")), "empty execution") + + assert.Zero(t, counter.execs, "no invalid record reached the database") + got, err := store.PriorAttempts(ctx, attemptToken) + require.NoError(t, err) + assert.Empty(t, got, "no invalid record reached the table") +} + +func TestPriorAttempts_AcrossExecutions(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + + got, err := store.PriorAttempts(ctx, attemptToken) + require.NoError(t, err) + assert.Empty(t, got, "no rows is not an error") + + require.NoError(t, store.RecordAttempt(ctx, attemptRec("exec-a", common.AttemptSending, "", "off-1"))) + require.NoError(t, store.RecordAttempt(ctx, attemptRec("exec-b", common.AttemptPurchased, "rn-9", "off-1"))) + other := attemptRec("exec-c", common.AttemptSending, "", "") + other.IdempotencyToken = "other-token-0123456" + require.NoError(t, store.RecordAttempt(ctx, other)) + + got, err = store.PriorAttempts(ctx, attemptToken) + require.NoError(t, err) + assert.ElementsMatch(t, []common.AttemptRecord{ + attemptRec("exec-a", common.AttemptSending, "", "off-1"), + attemptRec("exec-b", common.AttemptPurchased, "rn-9", "off-1"), + }, got) +} + +func TestRecordAttempt_OfferingIDWriteOnce(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + + require.NoError(t, store.RecordAttempt(ctx, attemptRec("exec-1", common.AttemptSending, "", "off-first"))) + require.NoError(t, store.RecordAttempt(ctx, attemptRec("exec-1", common.AttemptPurchased, "rn-1", "off-second"))) + got, err := store.PriorAttempts(ctx, attemptToken) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, "off-first", got[0].OfferingID, "first offering survives a later write") + + // A row first written without an offering takes the first one offered. + require.NoError(t, store.RecordAttempt(ctx, attemptRec("exec-2", common.AttemptSending, "", ""))) + require.NoError(t, store.RecordAttempt(ctx, attemptRec("exec-2", common.AttemptOutcomeUnknown, "", "off-late"))) + got, err = store.PriorAttempts(ctx, attemptToken) + require.NoError(t, err) + for _, r := range got { + if r.ExecutionID == "exec-2" { + assert.Equal(t, "off-late", r.OfferingID) + } + } +} diff --git a/internal/database/postgres/migrations/000158_purchase_attempts.down.sql b/internal/database/postgres/migrations/000158_purchase_attempts.down.sql new file mode 100644 index 000000000..24293ce86 --- /dev/null +++ b/internal/database/postgres/migrations/000158_purchase_attempts.down.sql @@ -0,0 +1,4 @@ +-- Reverse 000158: drop the purchase attempt journal (cloud-commitments-go#107). +-- Lossy by design: recorded attempts are dropped, so a re-dispatch falls back +-- to the provider's own window dedupe. +DROP TABLE IF EXISTS purchase_attempts; diff --git a/internal/database/postgres/migrations/000158_purchase_attempts.up.sql b/internal/database/postgres/migrations/000158_purchase_attempts.up.sql new file mode 100644 index 000000000..649743958 --- /dev/null +++ b/internal/database/postgres/migrations/000158_purchase_attempts.up.sql @@ -0,0 +1,30 @@ +-- Migration 000158: durable purchase attempt journal (cloud-commitments-go#107). +-- +-- One row per (idempotency token, execution): the Redshift provider records +-- "sending" immediately before the AWS call and "purchased" or +-- "outcome_unknown" after it, so a later attempt of the same token can adopt a +-- recorded reservation instead of buying twice. The store +-- (RecordAttempt in internal/config/store_postgres_attempts.go) enforces the +-- transition table in its upsert; this table only pins the value set. +-- +-- state MUST match common.AttemptSending / AttemptPurchased / +-- AttemptOutcomeUnknown in cloud-commitments-go +-- pkg/common/attempt_journal.go byte for byte. +-- reservation_id the provider commitment ID; NULL unless state = 'purchased'. +-- offering_id the offering the attempt was sent under; NULL when unknown, +-- written once (the upsert keeps the first non-NULL value). +CREATE TABLE IF NOT EXISTS purchase_attempts ( + idempotency_token TEXT NOT NULL CHECK (idempotency_token <> ''), + execution_id TEXT NOT NULL CHECK (execution_id <> ''), + state TEXT NOT NULL CHECK (state IN ('sending', 'purchased', 'outcome_unknown')), + reservation_id TEXT NULL, + offering_id TEXT NULL, + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT now(), + PRIMARY KEY (idempotency_token, execution_id), + CONSTRAINT purchase_attempts_reservation_iff_purchased + CHECK ((state = 'purchased') = (reservation_id IS NOT NULL)) +); + +CREATE INDEX IF NOT EXISTS idx_purchase_attempts_token + ON purchase_attempts (idempotency_token); diff --git a/internal/database/postgres/migrations/000158_purchase_attempts_test.go b/internal/database/postgres/migrations/000158_purchase_attempts_test.go new file mode 100644 index 000000000..b8f440c61 --- /dev/null +++ b/internal/database/postgres/migrations/000158_purchase_attempts_test.go @@ -0,0 +1,50 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Migration 000158 creates purchase_attempts; the down migration drops it. +func TestMigration_0158_PurchaseAttempts(t *testing.T) { + t.Parallel() + ctx := context.Background() + migrationsPath := getMigrationsPath() + + container := testhelpers.RequireEmptyDatabase(ctx, t) + pool := container.DB.Pool() + + tableExists := func() bool { + var exists bool + require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS ( + SELECT 1 FROM information_schema.tables WHERE table_name = 'purchase_attempts')`).Scan(&exists)) + return exists + } + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 158))) + assert.False(t, tableExists(), "table absent before 000158") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 158)) + assert.True(t, tableExists(), "table present after 000158") + + _, err := pool.Exec(ctx, `INSERT INTO purchase_attempts (idempotency_token, execution_id, state) VALUES ('t', 'e', 'bogus')`) + require.Error(t, err, "state CHECK rejects unknown states") + _, err = pool.Exec(ctx, `INSERT INTO purchase_attempts (idempotency_token, execution_id, state, reservation_id) VALUES ('t', 'e', 'sending', 'rn-1')`) + require.Error(t, err, "reservation_id is only valid when purchased") + _, err = pool.Exec(ctx, `INSERT INTO purchase_attempts (idempotency_token, execution_id, state) VALUES ('t', 'e', 'purchased')`) + require.Error(t, err, "purchased requires a reservation_id") + _, err = pool.Exec(ctx, `INSERT INTO purchase_attempts (idempotency_token, execution_id, state) VALUES ('t', 'e', 'sending')`) + require.NoError(t, err) + _, err = pool.Exec(ctx, `INSERT INTO purchase_attempts (idempotency_token, execution_id, state) VALUES ('t', 'e', 'sending')`) + require.Error(t, err, "primary key (token, execution)") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 158))) + assert.False(t, tableExists(), "the down migration drops the table") +} From 97821ff60f1d5ea5c1667a10f2da55d045982d94 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 21:11:29 +0100 Subject: [PATCH 2/5] feat(purchase): pass the attempt journal to each purchase ManagerConfig gains AttemptJournal, set to the config store by the post-DB Manager config, and each per-rec dispatch passes it in PurchaseOptions. Part of LeanerCloud/cloud-commitments-go#107 Co-Authored-By: Claude Sonnet 5.5 --- internal/purchase/execution.go | 1 + internal/purchase/manager.go | 5 +++ internal/purchase/requested_at_test.go | 42 ++++++++++++++++++++++ internal/server/app.go | 44 +++++++++++++---------- internal/server/app_postdb_config_test.go | 19 ++++++++++ 5 files changed, 93 insertions(+), 18 deletions(-) create mode 100644 internal/server/app_postdb_config_test.go diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 0a6af0320..7c039098f 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -831,6 +831,7 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf // mutation across goroutines). recOpts := opts recOpts.IdempotencyToken = common.DeriveIdempotencyToken(idempotencyLineageKey(exec), i) + recOpts.AttemptJournal = m.attemptJournal purchaseResult, err := m.executeSinglePurchase(ctx, rec, provCfg, recOpts) return recPurchaseOutcome{index: i, purchase: purchaseResult, err: err}, nil }, getMaxAccountParallelism()) diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index ab9625c90..41f9a2688 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -50,6 +50,9 @@ type ManagerConfig struct { // sweeps may still buy a row (issue #712); an older row is failed // unbought instead. Zero means DefaultMaxDueRowAge. MaxDueRowAge time.Duration + // AttemptJournal records each purchase attempt durably (go#107). Nil keeps + // the provider's own window dedupe. + AttemptJournal common.AttemptJournal } // Manager handles purchase workflow. @@ -61,6 +64,7 @@ type Manager struct { bastionSTS credentials.STSClientFactory ambientAWSCreds aws.CredentialsProvider credStore credentials.CredentialStore + attemptJournal common.AttemptJournal providerFactory provider.FactoryInterface oidcSigner oidc.Signer defaults PurchaseDefaults @@ -131,6 +135,7 @@ func NewManager(cfg ManagerConfig) *Manager { bastionSTS: cfg.BastionSTSFactory, ambientAWSCreds: cfg.AmbientAWSCreds, credStore: cfg.CredentialStore, + attemptJournal: cfg.AttemptJournal, providerFactory: factory, notifyDays: cfg.NotificationDaysBefore, defaults: PurchaseDefaults{ diff --git a/internal/purchase/requested_at_test.go b/internal/purchase/requested_at_test.go index 24eb23b77..611577428 100644 --- a/internal/purchase/requested_at_test.go +++ b/internal/purchase/requested_at_test.go @@ -17,6 +17,13 @@ import ( // runFanOutCapturingOpts runs a two-account fan-out of recs built by mk and // returns the PurchaseOptions every PurchaseCommitment call received. func runFanOutCapturingOpts(t *testing.T, service common.ServiceType, mk func(string) config.RecommendationRecord) []common.PurchaseOptions { + t.Helper() + return runFanOutCapturingOptsWithJournal(t, service, mk, nil) +} + +// runFanOutCapturingOptsWithJournal is runFanOutCapturingOpts on a Manager +// whose attempt journal is journal. +func runFanOutCapturingOptsWithJournal(t *testing.T, service common.ServiceType, mk func(string) config.RecommendationRecord, journal common.AttemptJournal) []common.PurchaseOptions { t.Helper() ctx := context.Background() accounts := []config.CloudAccount{ @@ -56,6 +63,7 @@ func runFanOutCapturingOpts(t *testing.T, service common.ServiceType, mk func(st manager := &Manager{ config: mockStore, email: mockEmail, providerFactory: mockFactory, credStore: awsAccessKeyCredStore(), dashboardURL: "https://dashboard.example.com", + attemptJournal: journal, } require.NoError(t, manager.executeMultiAccount(ctx, exec, &config.PurchasePlan{ID: "plan-x", Name: "Plan X"}, accounts)) return got @@ -360,3 +368,37 @@ func TestExecuteSinglePurchase_RedshiftWithoutRequestedAtRefuses(t *testing.T) { require.Error(t, err) mockServiceClient.AssertNotCalled(t, "PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything) } + +// fakeAttemptJournal is a never-called journal; the test only checks identity. +type fakeAttemptJournal struct{} + +func (*fakeAttemptJournal) PriorAttempts(context.Context, string) ([]common.AttemptRecord, error) { + return nil, nil +} +func (*fakeAttemptJournal) RecordAttempt(context.Context, common.AttemptRecord) error { return nil } + +// The journal reaches every per-rec dispatch together with the per-account +// ExecutionID the provider keys its attempt rows on (go#107). +func TestExecuteMultiAccount_PassesAttemptJournalAndPerAccountExecutionID(t *testing.T) { + journal := &fakeAttemptJournal{} + got := runFanOutCapturingOptsWithJournal(t, common.ServiceRedshift, func(acct string) config.RecommendationRecord { + r := scopedTestRec(acct) + r.Service, r.ResourceType = "redshift", "ra3.xlplus" + return r + }, journal) + require.Len(t, got, 2) + seen := map[string]bool{} + for _, o := range got { + assert.Same(t, journal, o.AttemptJournal) + assert.NotEmpty(t, o.ExecutionID) + assert.NotEqual(t, "root-exec", o.ExecutionID, "per-account row ID, not the root's") + seen[o.ExecutionID] = true + } + assert.Len(t, seen, 2, "each account dispatches under its own ExecutionID") +} + +func TestExecuteMultiAccount_NilJournalStaysNil(t *testing.T) { + for _, o := range runFanOutCapturingOpts(t, common.ServiceEC2, scopedTestRec) { + assert.Nil(t, o.AttemptJournal) + } +} diff --git a/internal/server/app.go b/internal/server/app.go index c50f913ec..04b32f66a 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -860,6 +860,31 @@ func (app *Application) startRateLimiterCleanup(rl cleanupWorkerStarter) { rl.StartCleanupWorker(workerCtx) } +// postDBPurchaseManagerConfig is the purchase Manager config once the database +// is connected. pgStore is the attempt journal (cloud-commitments-go#107); the +// pre-DB Manager leaves it nil on purpose and is replaced by this one. +func postDBPurchaseManagerConfig(app *Application, pgStore *config.PostgresStore, credStore credentials.CredentialStore, awsCfg aws.Config) purchase.ManagerConfig { + return purchase.ManagerConfig{ + ConfigStore: app.Config, + EmailSender: app.Email, + STSClient: sts.NewFromConfig(awsCfg), + AssumeRoleSTS: sts.NewFromConfig(awsCfg), + BastionSTSFactory: credentials.NewSTSClientFactory(awsCfg), + AmbientAWSCreds: awsCfg.Credentials, + CredentialStore: credStore, + AttemptJournal: pgStore, + NotificationDaysBefore: app.appConfig.NotificationDaysBefore, + MaxDueRowAge: app.appConfig.ScheduledPurchaseMaxOverdue, + DefaultTerm: app.appConfig.DefaultTerm, + DefaultPaymentOption: app.appConfig.DefaultPaymentOption, + DefaultCoverage: app.appConfig.DefaultCoverage, + DefaultRampSchedule: app.appConfig.DefaultRampSchedule, + DashboardURL: app.appConfig.DashboardURL, + OIDCSigner: app.signer, + OIDCIssuerURL: resolveOIDCIssuerURL(app.appConfig), + } +} + // reinitializeAfterConnect re-creates all stores and services that depend on the // database connection. This is called after the lazy DB connect succeeds. // Returns an error if any store or service initialization fails. @@ -926,24 +951,7 @@ func (app *Application) reinitializeAfterConnect(ctx context.Context, dbConn *da if err != nil { return fmt.Errorf("failed to load AWS config for cross-account STS: %w", err) } - reinitPurchase := purchase.NewManager(purchase.ManagerConfig{ - ConfigStore: app.Config, - EmailSender: app.Email, - STSClient: sts.NewFromConfig(awsCfg), - AssumeRoleSTS: sts.NewFromConfig(awsCfg), - BastionSTSFactory: credentials.NewSTSClientFactory(awsCfg), - AmbientAWSCreds: awsCfg.Credentials, - CredentialStore: credStore, - NotificationDaysBefore: app.appConfig.NotificationDaysBefore, - MaxDueRowAge: app.appConfig.ScheduledPurchaseMaxOverdue, - DefaultTerm: app.appConfig.DefaultTerm, - DefaultPaymentOption: app.appConfig.DefaultPaymentOption, - DefaultCoverage: app.appConfig.DefaultCoverage, - DefaultRampSchedule: app.appConfig.DefaultRampSchedule, - DashboardURL: app.appConfig.DashboardURL, - OIDCSigner: app.signer, - OIDCIssuerURL: resolveOIDCIssuerURL(app.appConfig), - }) + reinitPurchase := purchase.NewManager(postDBPurchaseManagerConfig(app, pgStore, credStore, awsCfg)) app.Purchase = reinitPurchase log.Println("Re-initialized purchase manager with credential store and cross-account STS") diff --git a/internal/server/app_postdb_config_test.go b/internal/server/app_postdb_config_test.go new file mode 100644 index 000000000..76aecd0b1 --- /dev/null +++ b/internal/server/app_postdb_config_test.go @@ -0,0 +1,19 @@ +package server + +import ( + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/stretchr/testify/assert" +) + +func TestPostDBPurchaseManagerConfig_SetsAttemptJournal(t *testing.T) { + pgStore := &config.PostgresStore{} + app := &Application{Config: pgStore} + + cfg := postDBPurchaseManagerConfig(app, pgStore, nil, aws.Config{}) + + assert.Same(t, pgStore, cfg.AttemptJournal, "post-DB manager journals attempts in the config store") + assert.Same(t, pgStore, cfg.ConfigStore) +} From 637cf4f50f076cdddd4c67d6df9e05eab21961e2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 21:11:40 +0100 Subject: [PATCH 3/5] feat(purchase): surface a bought-but-unrecorded reservation A failed rec that still carries a commitment ID keeps it as PurchaseID. The failure email switches to the outcome-unknown variant and names the node, and the failure log marker lists the IDs. Part of LeanerCloud/cloud-commitments-go#107 Co-Authored-By: Claude Sonnet 5.5 --- internal/email/purchase_outcome_unknown.go | 7 +- .../email/purchase_outcome_unknown_test.go | 34 ++++++++++ internal/email/sender.go | 8 +++ .../aggregate_unrecorded_commitment_test.go | 52 +++++++++++++++ internal/purchase/execution.go | 7 ++ internal/purchase/failure_notify.go | 36 ++++++++-- internal/purchase/failure_notify_test.go | 66 +++++++++++++++++++ .../purchase/failure_notify_writers_test.go | 20 ++++++ 8 files changed, 222 insertions(+), 8 deletions(-) create mode 100644 internal/purchase/aggregate_unrecorded_commitment_test.go diff --git a/internal/email/purchase_outcome_unknown.go b/internal/email/purchase_outcome_unknown.go index c156f93f8..030321898 100644 --- a/internal/email/purchase_outcome_unknown.go +++ b/internal/email/purchase_outcome_unknown.go @@ -13,8 +13,8 @@ const purchaseOutcomeUnknownSubject = "CUDly - Purchase Outcome Unknown - Verify const purchaseOutcomeUnknownTemplate = `CUDly - Purchase Outcome Unknown ================================ -The purchase run for execution {{.ExecutionID}}{{if .PlanID}} (plan {{.PlanID}}){{end}} was interrupted -before it recorded a result, and a recovery sweep marked it failed. CUDly +The purchase run for execution {{.ExecutionID}}{{if .PlanID}} (plan {{.PlanID}}){{end}} {{if .UnrecordedCommitment}}ended because the provider returned a commitment that CUDly could not record.{{else}}was interrupted +before it recorded a result, and a recovery sweep marked it failed.{{end}} CUDly cannot tell whether the commitments below were bought: the cloud provider may already have created them. @@ -24,7 +24,8 @@ Buying it again without checking can create a duplicate commitment. Outcome unknown: {{range .Recommendations}} - {{.Count}}x {{.ResourceType}}{{if .Engine}} ({{.Engine}}){{end}} in {{.Region}} - Service: {{.Service}} + Service: {{.Service}}{{if .UnrecordedCommitmentID}} + Redshift reserved node {{.UnrecordedCommitmentID}} was bought but CUDly could not record it; verify it in the cloud provider's console before buying again.{{end}} {{end}} Execution details: diff --git a/internal/email/purchase_outcome_unknown_test.go b/internal/email/purchase_outcome_unknown_test.go index e7fe8e9e9..37fe8be46 100644 --- a/internal/email/purchase_outcome_unknown_test.go +++ b/internal/email/purchase_outcome_unknown_test.go @@ -86,3 +86,37 @@ func TestSMTPSender_SendPurchaseFailedNotification_OutcomeUnknown(t *testing.T) }) } } + +func unrecordedCommitmentData() NotificationData { + data := outcomeUnknownData() + data.UnrecordedCommitment = true + data.Recommendations = []RecommendationSummary{ + {Service: "redshift", ResourceType: "ra3.xlplus", Region: "us-east-1", Count: 2, UnrecordedCommitmentID: "rn-123"}, + {Service: "rds", ResourceType: "db.m5.large", Region: "us-west-2", Count: 1}, + } + return data +} + +// cloud-commitments-go#107: a commitment the provider returned but CUDly could +// not record is named, the opening says why, and the email never says "retry". +func TestRenderPurchaseFailedEmail_UnrecordedCommitment(t *testing.T) { + body, err := RenderPurchaseFailedEmail(unrecordedCommitmentData()) + require.NoError(t, err) + + assert.Contains(t, body, "execution exec-1 (plan plan-1) ended because the provider returned a commitment that CUDly could not record.") + assert.Contains(t, body, "Redshift reserved node rn-123 was bought but CUDly could not record it; verify it in the cloud provider's console before buying again.") + assert.Equal(t, 1, strings.Count(body, "Redshift reserved node"), "only the rec that carries an ID gets the line") + assert.NotContains(t, body, "recovery sweep") + assert.NotContains(t, body, "was interrupted") + assert.NotContains(t, strings.ToLower(body), "retry") +} + +func TestRenderPurchaseFailedEmail_UnrecordedCommitmentAbsent(t *testing.T) { + body, err := RenderPurchaseFailedEmail(outcomeUnknownData()) + require.NoError(t, err) + + assert.Contains(t, body, "recovery sweep") + assert.Contains(t, body, "was interrupted") + assert.NotContains(t, body, "could not record") + assert.NotContains(t, body, "Redshift reserved node") +} diff --git a/internal/email/sender.go b/internal/email/sender.go index 04ad347fc..66b8d67dc 100644 --- a/internal/email/sender.go +++ b/internal/email/sender.go @@ -614,6 +614,11 @@ type NotificationData struct { // interrupted: the commitment may already exist, so the email asks the // operator to verify in the cloud console instead of retrying (issue #1415). OutcomeUnknown bool + // UnrecordedCommitment, with OutcomeUnknown, says the provider returned a + // commitment CUDly could not record (a recommendation carries its ID in + // UnrecordedCommitmentID), instead of a recovery sweep having interrupted + // the run (cloud-commitments-go#107). + UnrecordedCommitment bool } // RecommendationSummary is a simplified recommendation for email display. @@ -628,6 +633,9 @@ type RecommendationSummary struct { MonthlySavings float64 Term int UpfrontCost float64 + // UnrecordedCommitmentID is a commitment the provider created but CUDly + // could not record; the outcome-unknown email names it. + UnrecordedCommitmentID string // MonthlyCost, Commitment, AvailabilityZone, Platform and Tenancy are the // per-row detail of a scheduled plan step. Commitment is upfront plus // monthly over the term, so a no-upfront (Azure monthly) row does not read diff --git a/internal/purchase/aggregate_unrecorded_commitment_test.go b/internal/purchase/aggregate_unrecorded_commitment_test.go new file mode 100644 index 000000000..146a43219 --- /dev/null +++ b/internal/purchase/aggregate_unrecorded_commitment_test.go @@ -0,0 +1,52 @@ +package purchase + +import ( + "context" + "errors" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/execution" + "github.com/aws/aws-sdk-go-v2/service/ec2/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func aggregateOne(t *testing.T, rec config.RecommendationRecord, out recPurchaseOutcome) config.RecommendationRecord { + t.Helper() + exec := &config.PurchaseExecution{ExecutionID: "exec-1", Recommendations: []config.RecommendationRecord{rec}} + m := &Manager{} + m.aggregatePurchaseOutcomes(context.Background(), exec, &config.PurchasePlan{}, "111111111111", + types.OfferingClassTypeStandard, []execution.Result[recPurchaseOutcome]{{Value: out}}) + return exec.Recommendations[0] +} + +// A failed purchase that still carries a commitment ID (the provider bought a +// node it could not record, go#107) keeps the ID, and stays Purchased=false. +func TestAggregatePurchaseOutcomes_FailedWithCommitmentIDKeepsPurchaseID(t *testing.T) { + got := aggregateOne(t, config.RecommendationRecord{Service: "redshift", ResourceType: "ra3.xlplus"}, + recPurchaseOutcome{ + purchase: common.PurchaseResult{CommitmentID: "rn-new"}, + err: errors.New("purchase failed: redshift reserved node rn-new was bought but not recorded"), + }) + assert.Equal(t, "rn-new", got.PurchaseID) + assert.False(t, got.Purchased) + assert.NotEmpty(t, got.Error) +} + +// A Retry successor copies the predecessor's rec verbatim; when it fails +// without an ID the predecessor's ID is still true (the node exists). +func TestAggregatePurchaseOutcomes_RetrySuccessorFailingWithoutIDKeepsPredecessorID(t *testing.T) { + got := aggregateOne(t, config.RecommendationRecord{Service: "redshift", PurchaseID: "rn-pred"}, + recPurchaseOutcome{err: errors.New("purchase failed: boom")}) + assert.Equal(t, "rn-pred", got.PurchaseID) + assert.False(t, got.Purchased) +} + +func TestAggregatePurchaseOutcomes_FailedWithoutIDLeavesPurchaseIDEmpty(t *testing.T) { + got := aggregateOne(t, config.RecommendationRecord{Service: "redshift"}, + recPurchaseOutcome{err: errors.New("purchase failed: boom")}) + require.Empty(t, got.PurchaseID) + assert.False(t, got.Purchased) +} diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 7c039098f..1965e0f85 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -879,6 +879,13 @@ func (m *Manager) aggregatePurchaseOutcomes(ctx context.Context, exec *config.Pu if v.err != nil { logging.Errorf("Failed to purchase %s: %v", rec.ResourceType, v.err) exec.Recommendations[i].Error = v.err.Error() + // The provider returned a commitment it could not record (go#107): + // keep its ID on the rec, still Purchased=false, so the failure + // email names it. A failure without an ID leaves any ID the rec + // already carried (a Retry predecessor's) untouched. + if v.purchase.CommitmentID != "" { + exec.Recommendations[i].PurchaseID = v.purchase.CommitmentID + } purchaseErrors = append(purchaseErrors, fmt.Sprintf("%s: %v", rec.ResourceType, v.err)) continue } diff --git a/internal/purchase/failure_notify.go b/internal/purchase/failure_notify.go index 61f532c48..3e334e86e 100644 --- a/internal/purchase/failure_notify.go +++ b/internal/purchase/failure_notify.go @@ -92,7 +92,8 @@ func (m *Manager) sendFailureNoticeUntil(ctx context.Context, exec *config.Purch ctx, cancel := ctxutil.DetachedWithDeadline(ctx, until) defer cancel() data := m.buildPurchaseFailedData(ctx, exec) - data.OutcomeUnknown = outcomeUnknown + // buildPurchaseFailedData may already have set it (an unrecorded commitment). + data.OutcomeUnknown = data.OutcomeUnknown || outcomeUnknown if err := m.email.SendPurchaseFailedNotification(ctx, data); err != nil { logging.Errorf("Failed to send purchase-failed notification for execution %s: %v", exec.ExecutionID, err) } @@ -145,13 +146,21 @@ func (m *Manager) buildPurchaseFailedData(ctx context.Context, exec *config.Purc if !rec.Selected || rec.Purchased { continue } - data.Recommendations = append(data.Recommendations, email.RecommendationSummary{ + summary := email.RecommendationSummary{ Service: rec.Service, ResourceType: rec.ResourceType, Engine: rec.Engine, Region: rec.Region, Count: rec.Count, - }) + } + if rec.PurchaseID != "" { + // A commitment exists that this rec's failure never recorded + // (cloud-commitments-go#107): the email must say verify, not retry. + summary.UnrecordedCommitmentID = rec.PurchaseID + data.OutcomeUnknown = true + data.UnrecordedCommitment = true + } + data.Recommendations = append(data.Recommendations, summary) } return data } @@ -162,11 +171,28 @@ func logExecutionFailure(exec *config.PurchaseExecution) bool { if exec.Status != "failed" && exec.Status != "partially_completed" { return false } - logging.Errorf("%s execution_id=%s plan_id=%s status=%s", - PurchaseFailedLogMarker, exec.ExecutionID, exec.PlanID, exec.Status) + suffix := "" + if ids := unrecordedCommitmentIDs(exec); len(ids) > 0 { + // Carries the IDs to deployments with no email sender (go#107). + suffix = " unrecorded_commitment_ids=" + strings.Join(ids, ",") + } + logging.Errorf("%s execution_id=%s plan_id=%s status=%s%s", + PurchaseFailedLogMarker, exec.ExecutionID, exec.PlanID, exec.Status, suffix) return true } +// unrecordedCommitmentIDs lists the commitments that exist at the provider but +// that a selected rec's failure never recorded as purchased. +func unrecordedCommitmentIDs(exec *config.PurchaseExecution) []string { + var ids []string + for i := range exec.Recommendations { + if rec := exec.Recommendations[i]; rec.Selected && !rec.Purchased && rec.PurchaseID != "" { + ids = append(ids, rec.PurchaseID) + } + } + return ids +} + // notifyRootFailure is executeAndFinalize's notification for the root row. A // fan-out root always logs the marker but sends no email: the per-account rows // email for themselves, and the root could not list the failed recs (it never diff --git a/internal/purchase/failure_notify_test.go b/internal/purchase/failure_notify_test.go index eb4b0354d..0dddbe9d5 100644 --- a/internal/purchase/failure_notify_test.go +++ b/internal/purchase/failure_notify_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "strings" "sync" "testing" "time" @@ -217,3 +218,68 @@ func TestMultiAccountFailureNotifiesOncePerFailedAccountNotRoot(t *testing.T) { assert.Equal(t, failureNotifyRecipient, sent[0].RecipientEmail) require.Len(t, sent[0].Recommendations, 1) } + +func unrecordedCommitmentExec() *config.PurchaseExecution { + return &config.PurchaseExecution{ + ExecutionID: "exec-1", PlanID: "plan-1", Status: "failed", + Recommendations: []config.RecommendationRecord{ + {Service: "redshift", ResourceType: "ra3.xlplus", Region: "us-east-1", Count: 2, Selected: true, PurchaseID: "rn-123", Error: "purchase failed: bought but not recorded"}, + {Service: "rds", ResourceType: "db.r5.large", Region: "eu-west-1", Count: 1, Selected: true, Error: "purchase failed"}, + }, + } +} + +// go#107: a failed rec that carries a commitment ID switches the failed email +// to the outcome-unknown variant and names the ID; a plain failure stays plain. +func TestNotifyExecutionFailure_UnrecordedCommitmentIsOutcomeUnknown(t *testing.T) { + store := new(MockConfigStore) + store.On("GetGlobalConfig", mock.Anything).Return(failureNotifyCfg(), nil) + sender := new(MockEmailSender) + assertMocksOnCleanup(t, store, sender) + rec := &failedEmailRecorder{} + rec.expect(sender) + m := &Manager{config: store, email: sender} + + m.notifyExecutionFailure(context.Background(), unrecordedCommitmentExec()) + + sent := rec.all() + require.Len(t, sent, 1) + assert.True(t, sent[0].OutcomeUnknown) + assert.True(t, sent[0].UnrecordedCommitment) + require.Len(t, sent[0].Recommendations, 2) + assert.Equal(t, "rn-123", sent[0].Recommendations[0].UnrecordedCommitmentID) + assert.Empty(t, sent[0].Recommendations[1].UnrecordedCommitmentID) +} + +func TestNotifyExecutionFailure_NoPurchaseIDStaysPlainFailure(t *testing.T) { + store := new(MockConfigStore) + store.On("GetGlobalConfig", mock.Anything).Return(failureNotifyCfg(), nil) + sender := new(MockEmailSender) + assertMocksOnCleanup(t, store, sender) + rec := &failedEmailRecorder{} + rec.expect(sender) + m := &Manager{config: store, email: sender} + + exec := unrecordedCommitmentExec() + exec.Recommendations[0].PurchaseID = "" + m.notifyExecutionFailure(context.Background(), exec) + + sent := rec.all() + require.Len(t, sent, 1) + assert.False(t, sent[0].OutcomeUnknown) + assert.False(t, sent[0].UnrecordedCommitment) + assert.Empty(t, sent[0].Recommendations[0].UnrecordedCommitmentID) +} + +// The rendered email for that execution names the node and never says "retry". +func TestNotifyExecutionFailure_UnrecordedCommitmentRendersVerifyNotRetry(t *testing.T) { + store := new(MockConfigStore) + store.On("GetGlobalConfig", mock.Anything).Return(failureNotifyCfg(), nil) + assertMocksOnCleanup(t, store) + m := &Manager{config: store} + data := m.buildPurchaseFailedData(context.Background(), unrecordedCommitmentExec()) + body, err := email.RenderPurchaseFailedEmail(data) + require.NoError(t, err) + assert.Contains(t, body, "Redshift reserved node rn-123 was bought but CUDly could not record it") + assert.NotContains(t, strings.ToLower(body), "retry") +} diff --git a/internal/purchase/failure_notify_writers_test.go b/internal/purchase/failure_notify_writers_test.go index de93bb68b..7dad7ded0 100644 --- a/internal/purchase/failure_notify_writers_test.go +++ b/internal/purchase/failure_notify_writers_test.go @@ -222,3 +222,23 @@ func TestFanOutAuditLossChildStillNotifiesAndRootAddsMarker(t *testing.T) { assert.Equal(t, []string{childID}, emailed, "one email, for the child, none for the root") assert.Equal(t, 1, logs.count("root-1195")) } + +// go#107: the marker carries the unrecorded commitment IDs so the alarm and the +// log name them even with no email sender; a plain failure keeps the old line. +func TestLogExecutionFailure_AppendsUnrecordedCommitmentIDs(t *testing.T) { + logs := captureMarkers(t) + exec := &config.PurchaseExecution{ + ExecutionID: "exec-u", PlanID: "plan-1", Status: "failed", + Recommendations: []config.RecommendationRecord{ + {Selected: true, PurchaseID: "rn-1"}, + {Selected: true, PurchaseID: "rn-2"}, + {Selected: true, Purchased: true, PurchaseID: "rn-done"}, + {Selected: true}, + }, + } + require.True(t, logExecutionFailure(exec)) + assert.Contains(t, logs.buf.String(), "execution_id=exec-u plan_id=plan-1 status=failed unrecorded_commitment_ids=rn-1,rn-2") + + logExecutionFailure(&config.PurchaseExecution{ExecutionID: "exec-p", PlanID: "plan-1", Status: "failed"}) + assert.NotContains(t, logs.buf.String(), "execution_id=exec-p plan_id=plan-1 status=failed unrecorded") +} From c2e87444344ff1402652c48d1f8093c0bd2d0823 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 21:53:25 +0100 Subject: [PATCH 4/5] fix(purchase): keep the unrecorded-commitment email on the verify template An unrecorded commitment now selects the outcome-unknown email on its own, so a caller that assigns OutcomeUnknown later cannot switch it back to the retry wording. The ID line names a non-Redshift commitment generically, and the log-marker test pins the exact ID list. Part of LeanerCloud/cloud-commitments-go#107 Co-Authored-By: Claude Sonnet 5.5 --- internal/email/purchase_outcome_unknown.go | 11 ++++++-- .../email/purchase_outcome_unknown_test.go | 27 +++++++++++++++++++ internal/email/template_renderers.go | 2 +- .../purchase/failure_notify_writers_test.go | 6 ++++- 4 files changed, 42 insertions(+), 4 deletions(-) diff --git a/internal/email/purchase_outcome_unknown.go b/internal/email/purchase_outcome_unknown.go index 030321898..16a53b3b4 100644 --- a/internal/email/purchase_outcome_unknown.go +++ b/internal/email/purchase_outcome_unknown.go @@ -25,7 +25,7 @@ Outcome unknown: {{range .Recommendations}} - {{.Count}}x {{.ResourceType}}{{if .Engine}} ({{.Engine}}){{end}} in {{.Region}} Service: {{.Service}}{{if .UnrecordedCommitmentID}} - Redshift reserved node {{.UnrecordedCommitmentID}} was bought but CUDly could not record it; verify it in the cloud provider's console before buying again.{{end}} + {{if eq .Service "redshift"}}Redshift reserved node{{else}}Commitment{{end}} {{.UnrecordedCommitmentID}} was bought but CUDly could not record it; verify it in the cloud provider's console before buying again.{{end}} {{end}} Execution details: @@ -38,8 +38,15 @@ This is an automated message from CUDly. // purchase-failed subject, or the outcome-unknown subject when data is for a // row whose purchase may already exist. func purchaseFailedSubject(data NotificationData, failedSubject string) string { - if data.OutcomeUnknown { + if data.outcomeUnknownVariant() { return purchaseOutcomeUnknownSubject } return failedSubject } + +// outcomeUnknownVariant reports whether the purchase-failed email is the +// outcome-unknown one. An unrecorded commitment implies it, so a caller that +// later assigns OutcomeUnknown cannot switch the email back to "retry". +func (d NotificationData) outcomeUnknownVariant() bool { + return d.OutcomeUnknown || d.UnrecordedCommitment +} diff --git a/internal/email/purchase_outcome_unknown_test.go b/internal/email/purchase_outcome_unknown_test.go index 37fe8be46..08a825c55 100644 --- a/internal/email/purchase_outcome_unknown_test.go +++ b/internal/email/purchase_outcome_unknown_test.go @@ -120,3 +120,30 @@ func TestRenderPurchaseFailedEmail_UnrecordedCommitmentAbsent(t *testing.T) { assert.NotContains(t, body, "could not record") assert.NotContains(t, body, "Redshift reserved node") } + +// A caller that resets OutcomeUnknown after building the data must not turn an +// unrecorded commitment back into the "retry" email. +func TestPurchaseFailedEmail_UnrecordedCommitmentImpliesOutcomeUnknown(t *testing.T) { + data := unrecordedCommitmentData() + data.OutcomeUnknown = false + + body, err := RenderPurchaseFailedEmail(data) + require.NoError(t, err) + assert.Contains(t, body, "Purchase Outcome Unknown") + assert.Contains(t, body, "Redshift reserved node rn-123 was bought but CUDly could not record it") + assert.NotContains(t, strings.ToLower(body), "retry") + assert.Equal(t, purchaseOutcomeUnknownSubject, purchaseFailedSubject(data, "CUDly Purchase Failed")) +} + +// The ID line names a non-Redshift commitment generically. +func TestRenderPurchaseFailedEmail_UnrecordedCommitmentNonRedshift(t *testing.T) { + data := unrecordedCommitmentData() + data.Recommendations = []RecommendationSummary{ + {Service: "rds", ResourceType: "db.m5.large", Region: "us-west-2", Count: 1, UnrecordedCommitmentID: "ri-77"}, + } + body, err := RenderPurchaseFailedEmail(data) + require.NoError(t, err) + assert.Contains(t, body, "Commitment ri-77 was bought but CUDly could not record it; verify it in the cloud provider's console before buying again.") + assert.NotContains(t, body, "Redshift") + assert.NotContains(t, strings.ToLower(body), "retry") +} diff --git a/internal/email/template_renderers.go b/internal/email/template_renderers.go index 7710d8946..05508c267 100644 --- a/internal/email/template_renderers.go +++ b/internal/email/template_renderers.go @@ -111,7 +111,7 @@ func RenderPurchaseConfirmationEmail(data NotificationData) (string, error) { // template, or its outcome-unknown variant when data.OutcomeUnknown is set // (issue #1415). func RenderPurchaseFailedEmail(data NotificationData) (string, error) { - if data.OutcomeUnknown { + if data.outcomeUnknownVariant() { return renderTextTemplate("outcome-unknown", purchaseOutcomeUnknownTemplate, data) } return renderTextTemplate("failed", purchaseFailedTemplate, data) diff --git a/internal/purchase/failure_notify_writers_test.go b/internal/purchase/failure_notify_writers_test.go index 7dad7ded0..f45444580 100644 --- a/internal/purchase/failure_notify_writers_test.go +++ b/internal/purchase/failure_notify_writers_test.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "regexp" "strings" "sync" "testing" @@ -234,10 +235,13 @@ func TestLogExecutionFailure_AppendsUnrecordedCommitmentIDs(t *testing.T) { {Selected: true, PurchaseID: "rn-2"}, {Selected: true, Purchased: true, PurchaseID: "rn-done"}, {Selected: true}, + {PurchaseID: "rn-unselected"}, }, } require.True(t, logExecutionFailure(exec)) - assert.Contains(t, logs.buf.String(), "execution_id=exec-u plan_id=plan-1 status=failed unrecorded_commitment_ids=rn-1,rn-2") + m := regexp.MustCompile(`execution_id=exec-u plan_id=plan-1 status=failed unrecorded_commitment_ids=(\S*)`).FindStringSubmatch(logs.buf.String()) + require.Len(t, m, 2) + assert.Equal(t, "rn-1,rn-2", m[1], "purchased and unselected recs are not listed") logExecutionFailure(&config.PurchaseExecution{ExecutionID: "exec-p", PlanID: "plan-1", Status: "failed"}) assert.NotContains(t, logs.buf.String(), "execution_id=exec-p plan_id=plan-1 status=failed unrecorded") From 924de98f5c6ac07f27a4f5d3384e18c9a824445e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 22:20:14 +0100 Subject: [PATCH 5/5] chore(db): renumber the purchase_attempts migration to 000161 Open PR #1239 takes 000158-000160, so this migration moves to 000161. Part of LeanerCloud/cloud-commitments-go#107 Co-Authored-By: Claude Opus 5.5 --- ....down.sql => 000161_purchase_attempts.down.sql} | 2 +- ...mpts.up.sql => 000161_purchase_attempts.up.sql} | 2 +- ...ts_test.go => 000161_purchase_attempts_test.go} | 14 +++++++------- 3 files changed, 9 insertions(+), 9 deletions(-) rename internal/database/postgres/migrations/{000158_purchase_attempts.down.sql => 000161_purchase_attempts.down.sql} (73%) rename internal/database/postgres/migrations/{000158_purchase_attempts.up.sql => 000161_purchase_attempts.up.sql} (96%) rename internal/database/postgres/migrations/{000158_purchase_attempts_test.go => 000161_purchase_attempts_test.go} (84%) diff --git a/internal/database/postgres/migrations/000158_purchase_attempts.down.sql b/internal/database/postgres/migrations/000161_purchase_attempts.down.sql similarity index 73% rename from internal/database/postgres/migrations/000158_purchase_attempts.down.sql rename to internal/database/postgres/migrations/000161_purchase_attempts.down.sql index 24293ce86..7cb57f457 100644 --- a/internal/database/postgres/migrations/000158_purchase_attempts.down.sql +++ b/internal/database/postgres/migrations/000161_purchase_attempts.down.sql @@ -1,4 +1,4 @@ --- Reverse 000158: drop the purchase attempt journal (cloud-commitments-go#107). +-- Reverse 000161: drop the purchase attempt journal (cloud-commitments-go#107). -- Lossy by design: recorded attempts are dropped, so a re-dispatch falls back -- to the provider's own window dedupe. DROP TABLE IF EXISTS purchase_attempts; diff --git a/internal/database/postgres/migrations/000158_purchase_attempts.up.sql b/internal/database/postgres/migrations/000161_purchase_attempts.up.sql similarity index 96% rename from internal/database/postgres/migrations/000158_purchase_attempts.up.sql rename to internal/database/postgres/migrations/000161_purchase_attempts.up.sql index 649743958..4dcf461b0 100644 --- a/internal/database/postgres/migrations/000158_purchase_attempts.up.sql +++ b/internal/database/postgres/migrations/000161_purchase_attempts.up.sql @@ -1,4 +1,4 @@ --- Migration 000158: durable purchase attempt journal (cloud-commitments-go#107). +-- Migration 000161: durable purchase attempt journal (cloud-commitments-go#107). -- -- One row per (idempotency token, execution): the Redshift provider records -- "sending" immediately before the AWS call and "purchased" or diff --git a/internal/database/postgres/migrations/000158_purchase_attempts_test.go b/internal/database/postgres/migrations/000161_purchase_attempts_test.go similarity index 84% rename from internal/database/postgres/migrations/000158_purchase_attempts_test.go rename to internal/database/postgres/migrations/000161_purchase_attempts_test.go index b8f440c61..13c9ae98b 100644 --- a/internal/database/postgres/migrations/000158_purchase_attempts_test.go +++ b/internal/database/postgres/migrations/000161_purchase_attempts_test.go @@ -12,8 +12,8 @@ import ( "github.com/stretchr/testify/require" ) -// Migration 000158 creates purchase_attempts; the down migration drops it. -func TestMigration_0158_PurchaseAttempts(t *testing.T) { +// Migration 000161 creates purchase_attempts; the down migration drops it. +func TestMigration_0161_PurchaseAttempts(t *testing.T) { t.Parallel() ctx := context.Background() migrationsPath := getMigrationsPath() @@ -28,11 +28,11 @@ func TestMigration_0158_PurchaseAttempts(t *testing.T) { return exists } - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 158))) - assert.False(t, tableExists(), "table absent before 000158") + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 161))) + assert.False(t, tableExists(), "table absent before 000161") - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 158)) - assert.True(t, tableExists(), "table present after 000158") + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 161)) + assert.True(t, tableExists(), "table present after 000161") _, err := pool.Exec(ctx, `INSERT INTO purchase_attempts (idempotency_token, execution_id, state) VALUES ('t', 'e', 'bogus')`) require.Error(t, err, "state CHECK rejects unknown states") @@ -45,6 +45,6 @@ func TestMigration_0158_PurchaseAttempts(t *testing.T) { _, err = pool.Exec(ctx, `INSERT INTO purchase_attempts (idempotency_token, execution_id, state) VALUES ('t', 'e', 'sending')`) require.Error(t, err, "primary key (token, execution)") - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 158))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 161))) assert.False(t, tableExists(), "the down migration drops the table") }