fix(purchase,server): skip the $0 purchase confirmation and report a failed lazy DB init on /health

internal/purchase/approvals_test.go

@@ -283,7 +283,9 @@ func TestManager_ApproveAndExecute_EmptyPlanID(t *testing.T) {
 	}
 	store.On("TransitionExecutionStatus", ctx, "exec-direct-1", approveFromStatuses, "running", (*string)(nil)).Return(updated, nil)
 	// No GetPurchasePlan / GetPlanAccounts / UpdatePurchasePlan calls — see AssertNotCalled below.
-	sender.On("SendPurchaseConfirmation", mock.Anything, mock.Anything).Return(nil)
+	// No recommendations, so nothing is bought and no "purchase confirmed"
+	// email goes out (#339): with no SendPurchaseConfirmation expectation
+	// registered, a send fails the test.
 	store.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil)
 
 	_, _, err := manager.ApproveAndExecute(ctx, "exec-direct-1", "operator@example.com", nil, nil)

internal/purchase/coverage_extra_test.go

@@ -412,7 +412,8 @@ func TestProcessMessage_ApproveHappyPath(t *testing.T) {
 	// mistake) would mismatch the mock expectation and fail the test.
 	plan := &config.PurchasePlan{ID: planID, Name: "test-plan"}
 	mockStore.On("GetPurchasePlan", mock.Anything, planID).Return(plan, nil)
-	mockEmail.On("SendPurchaseConfirmation", mock.Anything, mock.Anything).Return(nil)
+	// Nothing is selected, so nothing is bought and no "purchase confirmed"
+	// email goes out (#339).
 	mockStore.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil)
 	mockStore.On("CompletePlanStep", mock.Anything, planID, 1).Return(nil)
 
@@ -425,6 +426,7 @@ func TestProcessMessage_ApproveHappyPath(t *testing.T) {
 	err := manager.ProcessMessage(ctx, `{"type":"approve","execution_id":"exec-appv","token":"correct-token","actor_email":"owner@example.com"}`)
 	require.NoError(t, err)
 	mockStore.AssertExpectations(t)
+	mockEmail.AssertNotCalled(t, "SendPurchaseConfirmation", mock.Anything, mock.Anything)
 	mockEmail.AssertExpectations(t)
 }
 
@@ -585,7 +587,8 @@ func TestProcessMessage_ApproveFourEyesOn_DifferentApproverSucceeds(t *testing.T
 	mockStore.On("TransitionExecutionStatus", ctx, "exec-appv-diff", []string{"pending", "notified"}, "running", (*string)(nil)).Return(approved, nil)
 	plan := &config.PurchasePlan{ID: planID, Name: "test-plan"}
 	mockStore.On("GetPurchasePlan", mock.Anything, planID).Return(plan, nil)
-	mockEmail.On("SendPurchaseConfirmation", mock.Anything, mock.Anything).Return(nil)
+	// Nothing is selected, so nothing is bought and no "purchase confirmed"
+	// email goes out (#339).
 	mockStore.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil)
 	mockStore.On("CompletePlanStep", mock.Anything, planID, 1).Return(nil)
 
@@ -598,6 +601,7 @@ func TestProcessMessage_ApproveFourEyesOn_DifferentApproverSucceeds(t *testing.T
 	err := manager.ProcessMessage(ctx, `{"type":"approve","execution_id":"exec-appv-diff","token":"correct-token","actor_email":"approver@example.com"}`)
 	require.NoError(t, err)
 	mockStore.AssertExpectations(t)
+	mockEmail.AssertNotCalled(t, "SendPurchaseConfirmation", mock.Anything, mock.Anything)
 	mockEmail.AssertExpectations(t)
 }
 

internal/purchase/empty_selection_notify_test.go

@@ -0,0 +1,108 @@
+package purchase
+
+import (
+	"context"
+	"testing"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+// unselectedRecs is a plan step whose recommendations are all deselected, so
+// processPurchaseRecommendations short-circuits before any cloud API call.
+func unselectedRecs() []config.RecommendationRecord {
+	return []config.RecommendationRecord{
+		{Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Savings: 50},
+		{Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, UpfrontCost: 600, Savings: 90},
+	}
+}
+
+// TestExecutePurchase_SingleAccount_NothingSelected_SendsNoConfirmation is the
+// #339 (A05-015) regression guard for the single-account path: a run with
+// nothing selected buys nothing, so it must not email a "purchase confirmed"
+// for zero commitments and $0.
+func TestExecutePurchase_SingleAccount_NothingSelected_SendsNoConfirmation(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockConfigStore)
+	mockEmail := new(MockEmailSender)
+	mockFactory := new(MockProviderFactory)
+
+	plan := &config.PurchasePlan{ID: "plan-339", Name: "Plan 339"}
+	exec := &config.PurchaseExecution{
+		ExecutionID:     "exec-339-single",
+		PlanID:          "plan-339",
+		StepNumber:      1,
+		Recommendations: unselectedRecs(),
+	}
+	mockStore.On("GetPurchasePlan", mock.Anything, "plan-339").Return(plan, nil)
+	mockEmail.On("SendPurchaseConfirmation", mock.Anything, mock.Anything).Return(nil).Maybe()
+
+	manager := &Manager{
+		config:          mockStore,
+		email:           mockEmail,
+		providerFactory: mockFactory,
+		dashboardURL:    "https://dashboard.example.com",
+	}
+
+	require.NoError(t, manager.executePurchase(ctx, exec))
+
+	mockEmail.AssertNotCalled(t, "SendPurchaseConfirmation", mock.Anything, mock.Anything)
+	mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything)
+	for i := range exec.Recommendations {
+		assert.False(t, exec.Recommendations[i].Purchased)
+	}
+}
+
+// TestExecuteForAccount_NothingSelected_SendsNoConfirmation is the same #339
+// guard for the multi-account fan-out path, which gated the confirmation on
+// "no per-rec errors" and so also confirmed an account that bought nothing.
+func TestExecuteForAccount_NothingSelected_SendsNoConfirmation(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockConfigStore)
+	mockEmail := new(MockEmailSender)
+	allowFailedNotification(mockEmail)
+	mockFactory := new(MockProviderFactory)
+
+	const acctID = "aaaaaaaa-0000-0000-0000-000000000339"
+	account := config.CloudAccount{ID: acctID, Name: "Prod", Provider: "aws", ExternalID: "111111111111", AWSAuthMode: "access_keys"}
+	baseExec := &config.PurchaseExecution{
+		ExecutionID:     "exec-339-base",
+		PlanID:          "plan-339",
+		Source:          common.PurchaseSourceWeb,
+		Recommendations: unselectedRecs(),
+	}
+	plan := &config.PurchasePlan{ID: "plan-339", Name: "Plan 339"}
+
+	var savedExec *config.PurchaseExecution
+	mockStore.SavePurchaseExecutionFn = func(_ context.Context, e *config.PurchaseExecution) error {
+		c := *e
+		savedExec = &c
+		return nil
+	}
+	mockEmail.On("SendPurchaseConfirmation", mock.Anything, mock.Anything).Return(nil).Maybe()
+
+	credStore := &MockCredentialStore{
+		LoadRawFn: func(_ context.Context, _, _ string) ([]byte, error) {
+			return []byte(`{"access_key_id":"AKIAIOSFODNN7EXAMPLE","secret_access_key":"wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"}`), nil
+		},
+	}
+
+	manager := &Manager{
+		config:          mockStore,
+		email:           mockEmail,
+		providerFactory: mockFactory,
+		credStore:       credStore,
+		dashboardURL:    "https://dashboard.example.com",
+	}
+
+	committed, err := manager.executeForAccount(ctx, baseExec, plan, account)
+	require.NoError(t, err)
+	assert.False(t, committed, "nothing was selected, so nothing committed")
+
+	require.NotNil(t, savedExec, "the per-account row is still saved")
+	mockEmail.AssertNotCalled(t, "SendPurchaseConfirmation", mock.Anything, mock.Anything)
+	mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything)
+}

internal/purchase/execution.go

@@ -134,11 +134,14 @@ func (m *Manager) executeSingleAccount(ctx context.Context, exec *config.Purchas
 		return fmt.Errorf("some purchases failed: %v", purchaseErrors)
 	}
 
-	// At least one rec committed (full or partial success): the confirmation
-	// must go out for the recs that purchased. buildPurchaseConfirmationData
-	// only lists Purchased recs, so a partial run notifies on exactly those.
-	if notifyErr := m.sendPurchaseNotification(ctx, exec, plan, totalSavings, totalUpfront); notifyErr != nil {
-		logging.Errorf("Failed to send confirmation: %v", notifyErr)
+	// Confirm only when at least one rec committed (full or partial success).
+	// buildPurchaseConfirmationData only lists Purchased recs, so a partial run
+	// notifies on exactly those, and a run with nothing selected (it never
+	// reached a cloud API) sends no "purchase confirmed" email for $0 (#339).
+	if anyRecPurchased(exec.Recommendations) {
+		if notifyErr := m.sendPurchaseNotification(ctx, exec, plan, totalSavings, totalUpfront); notifyErr != nil {
+			logging.Errorf("Failed to send confirmation: %v", notifyErr)
+		}
 	}
 
 	if len(purchaseErrors) > 0 {
@@ -329,16 +332,18 @@ func (m *Manager) executeForAccount(ctx context.Context, baseExec *config.Purcha
 
 	// Resolve the final status/error/completion stamp from the per-rec outcome
 	// (#642), and learn whether any rec committed (#1014).
-	partial, committed := applyAccountOutcome(&acctExec, purchaseErrors)
+	_, committed = applyAccountOutcome(&acctExec, purchaseErrors)
 
 	if saveErr := m.config.SavePurchaseExecution(ctx, &acctExec); saveErr != nil {
 		return committed, fmt.Errorf("AUDIT LOSS: failed to save execution record for account %s: %w", account.ID, saveErr)
 	}
 
 	// Send the confirmation whenever at least one rec committed (full or
 	// partial success). buildPurchaseConfirmationData only lists Purchased
-	// recs, so a partial run notifies on exactly the recs that fired.
-	if len(purchaseErrors) == 0 || partial {
+	// recs, so a partial run notifies on exactly the recs that fired. Gating on
+	// committed rather than "no errors" keeps an account with nothing selected
+	// from getting a "purchase confirmed" email for $0 (#339).
+	if committed {
 		if err := m.sendPurchaseNotification(ctx, &acctExec, plan, totalSavings, totalUpfront); err != nil {
 			logging.Errorf("Failed to send confirmation for account %s: %v", account.ID, err)
 		}

internal/server/health.go

@@ -99,10 +99,10 @@ func (app *Application) collectHealth(ctx context.Context) HealthStatus {
 	// pointers when the mutex is free, and while initialization holds it
 	// report both stores as pending rather than race on half-written state.
 	if app.dbMu.TryLock() {
-		configStore, authService, db := app.Config, app.Auth, app.DB
+		configStore, authService, db, connected := app.Config, app.Auth, app.DB, app.dbConnected
 		app.dbMu.Unlock()
 
-		health.Checks["config_store"] = app.checkConfigStore(ctx, configStore, db)
+		health.Checks["config_store"] = app.checkConfigStore(ctx, configStore, db, connected)
 		if health.Checks["config_store"].Status != "healthy" {
 			health.Status = "degraded"
 		}
@@ -169,10 +169,10 @@ func (app *Application) checkMigrations() CheckResult {
 	}
 }
 
-// checkConfigStore checks the health of the configuration store. The store and
-// connection come in as a snapshot taken under dbMu by collectHealth, so this
-// function never reads pointers ensureDB may be rewriting.
-func (app *Application) checkConfigStore(ctx context.Context, configStore config.StoreInterface, db *database.Connection) CheckResult {
+// checkConfigStore checks the health of the configuration store. The store,
+// connection and connected flag come in as a snapshot taken under dbMu by
+// collectHealth, so this function never reads state ensureDB may be rewriting.
+func (app *Application) checkConfigStore(ctx context.Context, configStore config.StoreInterface, db *database.Connection, connected bool) CheckResult {
 	// Check if config store exists
 	if configStore == nil {
 		// If using PostgreSQL with lazy initialization, DB might not be connected yet
@@ -188,6 +188,18 @@ func (app *Application) checkConfigStore(ctx context.Context, configStore config
 		}
 	}
 
+	// With lazy PostgreSQL init the store is only usable once ensureDB has
+	// finished. reinitializeAfterConnect sets app.Config before steps that can
+	// still fail; ensureDB then closes the pool and clears app.DB but leaves
+	// app.Config over the closed pool. Without this check that state reported
+	// healthy (#339). ensureDB retries on the next request.
+	if app.dbConfig != nil && !connected {
+		return CheckResult{
+			Status:  "unhealthy",
+			Message: "Database initialization did not complete; config store is not connected",
+		}
+	}
+
 	// If using PostgreSQL, check database connection health
 	if db != nil {
 		if err := db.HealthCheck(ctx); err != nil {

internal/server/health_reinit_failure_integration_test.go

@@ -0,0 +1,64 @@
+//go:build integration
+
+package server
+
+import (
+	"context"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestHealth_ConfigStoreAfterFailedReinitialization is the #339 (A06-009)
+// regression test on the real path: ensureDB connects to a live Postgres,
+// reinitializeAfterConnect sets app.Config and then fails (here at the AWS
+// config load, via a profile that does not exist), and ensureDB closes the
+// pool. The config_store check used to report "healthy" over that closed pool.
+func TestHealth_ConfigStoreAfterFailedReinitialization(t *testing.T) {
+	for _, key := range []string{
+		"CREDENTIAL_ENCRYPTION_KEY_SECRET_ARN", "CREDENTIAL_ENCRYPTION_KEY_SECRET_NAME",
+		"CREDENTIAL_ENCRYPTION_KEY_SECRET_ID", "CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY",
+		"CUDLY_SIGNING_KEY_ID", "CUDLY_SIGNING_KEY_VAULT_URL", "CUDLY_SIGNING_KEY_NAME",
+		"CUDLY_SIGNING_KEY_RESOURCE", "AWS_LAMBDA_RUNTIME_API",
+	} {
+		t.Setenv(key, "")
+	}
+	t.Setenv("CREDENTIAL_ENCRYPTION_KEY", strings.Repeat("12", 32))
+	t.Setenv("CUDLY_ISSUER_URL", "https://health-339.example.test")
+	t.Setenv("CUDLY_SOURCE_CLOUD", "aws")
+	t.Setenv("SCHEDULED_TASK_AUTH_MODE", "disabled")
+	t.Setenv("AWS_EC2_METADATA_DISABLED", "true")
+	t.Setenv("AWS_CONFIG_FILE", "/dev/null")
+	t.Setenv("AWS_SHARED_CREDENTIALS_FILE", "/dev/null")
+	// LoadDefaultConfig fails on a named profile missing from the config file,
+	// after reinitializeAfterConnect has already published app.Config.
+	t.Setenv("AWS_PROFILE", "issue-339-missing-profile")
+
+	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+	defer cancel()
+	pg, err := testhelpers.SetupPostgresContainer(ctx, t)
+	require.NoError(t, err)
+	t.Cleanup(func() { require.NoError(t, pg.Cleanup(context.Background())) })
+
+	app, err := NewApplicationFromDeps(ctx, ApplicationConfig{DashboardURL: "https://health-339.example.test"},
+		ExternalDeps{DBConfig: pg.Config, EmailSender: &noopEmailSender{}})
+	require.NoError(t, err)
+	t.Cleanup(func() { require.NoError(t, app.Close()) })
+
+	err = app.ensureDB(ctx)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "failed to reinitialize after DB connect")
+	require.NotNil(t, app.Config, "the failure path leaves app.Config over the closed pool")
+	require.Nil(t, app.DB)
+	require.False(t, app.dbConnected)
+
+	health := app.collectHealth(ctx)
+	assert.Equal(t, "unhealthy", health.Checks["config_store"].Status,
+		"config_store must not report healthy over a store whose pool ensureDB closed")
+	assert.Equal(t, "degraded", health.Status)
+	assert.False(t, health.ready())
+}

internal/server/health_test.go

@@ -368,6 +368,25 @@ func TestCheckConfigStore(t *testing.T) {
 			},
 			expectedStatus: "pending",
 		},
+		{
+			// #339: reinitializeAfterConnect set Config, then failed, and
+			// ensureDB closed the pool and cleared DB without connecting.
+			name: "config store left over a failed lazy init",
+			setupApp: func(app *Application) {
+				app.Config = &mockConfigStoreForHealth{}
+				app.dbConfig = &databaseConfigStub{}
+			},
+			expectedStatus: "unhealthy",
+		},
+		{
+			name: "config store after a completed lazy init",
+			setupApp: func(app *Application) {
+				app.Config = &mockConfigStoreForHealth{}
+				app.dbConfig = &databaseConfigStub{}
+				app.dbConnected = true
+			},
+			expectedStatus: "healthy",
+		},
 	}
 
 	for _, tt := range tests {
@@ -378,7 +397,7 @@ func TestCheckConfigStore(t *testing.T) {
 				tt.setupApp(app)
 			}
 
-			result := app.checkConfigStore(ctx, app.Config, app.DB)
+			result := app.checkConfigStore(ctx, app.Config, app.DB, app.dbConnected)
 			testutil.AssertEqual(t, tt.expectedStatus, result.Status)
 		})
 	}

internal/server/readiness_test.go

@@ -98,6 +98,8 @@ func TestReadinessFailedMigrations(t *testing.T) {
 		Config:   &mockConfigStoreForHealth{},
 		Auth:     createHealthyAuthService(),
 		dbConfig: &dbConfigForReadiness,
+		// ensureDB finished: the stores above are the live ones.
+		dbConnected: true,
 	}
 	app.recordMigrationResult(context.DeadlineExceeded)
 
@@ -120,6 +122,8 @@ func TestReadinessPendingMigrations(t *testing.T) {
 		Config:   &mockConfigStoreForHealth{},
 		Auth:     createHealthyAuthService(),
 		dbConfig: &dbConfigForReadiness,
+		// ensureDB finished: the stores above are the live ones.
+		dbConnected: true,
 	}
 
 	req := httptest.NewRequestWithContext(context.Background(), "GET", "/ready", nil)
@@ -145,6 +149,8 @@ func TestReadinessDisabledMigrationsStayReady(t *testing.T) {
 		Config:   &mockConfigStoreForHealth{},
 		Auth:     createHealthyAuthService(),
 		dbConfig: &noMigrate,
+		// ensureDB finished: the stores above are the live ones.
+		dbConnected: true,
 	}
 
 	req := httptest.NewRequestWithContext(context.Background(), "GET", "/ready", nil)