From efd9bd3dbc547c8b00768bdde39d86ff57bfdf3e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 16:08:54 +0200 Subject: [PATCH 1/3] fix(purchase): attach recommendations to explicit manual plan steps Allow enabled manual plans to attach and notify explicit scheduled roots while automatic purchasing is disabled. Preserve automatic mode guards and optional manual commitment caps. Fail due empty manual roots without requiring an email recipient or overdue grace eligibility, pinning the empty recommendation snapshot so concurrent attachment cannot be overwritten. Refs #631 --- .../manual_plan_step_integration_test.go | 108 ++++++++++++++++++ internal/purchase/notifications.go | 50 +++++++- internal/purchase/notifications_test.go | 5 +- internal/purchase/plan_step_atomic_test.go | 2 +- internal/purchase/plan_step_attach.go | 15 ++- internal/purchase/plan_step_attach_test.go | 102 +++++++++++++++++ internal/purchase/step_rows_test.go | 3 +- 7 files changed, 274 insertions(+), 11 deletions(-) create mode 100644 internal/purchase/manual_plan_step_integration_test.go diff --git a/internal/purchase/manual_plan_step_integration_test.go b/internal/purchase/manual_plan_step_integration_test.go new file mode 100644 index 00000000..751050a8 --- /dev/null +++ b/internal/purchase/manual_plan_step_integration_test.go @@ -0,0 +1,108 @@ +//go:build integration + +package purchase + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real PostgreSQL and resolver; recommendation reads and email delivery are fixtures. +func TestManualPlanStep_AzureSizingPersistsWithoutAutoPurchase(t *testing.T) { + ctx := context.Background() + store, pool := rampStepStore(ctx, t) + global, err := store.GetGlobalConfig(ctx) + require.NoError(t, err) + recipient := "fixture@example.invalid" + global.NotificationEmail, global.AutoPurchaseEnabled = &recipient, false + require.NoError(t, store.SaveGlobalConfig(ctx, global)) + account := &config.CloudAccount{Name: "manual-azure", Provider: "azure", ExternalID: uuid.NewString(), Enabled: true} + require.NoError(t, store.CreateCloudAccount(ctx, account)) + next := time.Now().Add(84 * time.Hour).Truncate(time.Microsecond) + plan := &config.PurchasePlan{Name: "manual-azure", Enabled: true, NotificationDaysBefore: 3, + Services: map[string]config.ServiceConfig{"azure:compute": {Provider: "azure", Service: "compute", Enabled: true, Term: 1, Payment: "monthly", Coverage: 80}}, + RampSchedule: config.RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4, StartDate: next}, + NextExecutionDate: &next} + require.NoError(t, plan.Validate()) + require.NoError(t, store.CreatePurchasePlan(ctx, plan)) + require.NoError(t, store.SetPlanAccounts(ctx, plan.ID, []string{account.ID})) + monthly := 1000.0 + rec := config.RecommendationRecord{ID: "manual-azure-rec", Provider: "azure", Service: "compute", Region: "westeurope", ResourceType: "Standard_D2s_v3", + Count: 100, Term: 1, Payment: "monthly", MonthlyCost: &monthly, Savings: 500, CloudAccountID: &account.ID} + sender := newCountingSender(ctx) + m := &Manager{config: store, email: sender, resolver: planstepResolver{ + src: &cumSource{shown: func() []config.RecommendationRecord { return []config.RecommendationRecord{rec} }}, store: cumStore{rows: []config.RecommendationRecord{rec}}}} + result, err := m.SendUpcomingPurchaseNotifications(ctx) + require.NoError(t, err) + assert.Zero(t, result.Notified, "manual plans do not implicitly mint roots") + assert.Empty(t, stepRowsInDB(ctx, t, store, plan.ID)) + root := &config.PurchaseExecution{ExecutionID: uuid.NewString(), PlanID: plan.ID, Status: "pending", StepNumber: 1, ScheduledDate: next} + require.NoError(t, store.SavePurchaseExecution(ctx, root)) + var wg sync.WaitGroup + errors := make(chan error, 4) + for range 4 { + wg.Add(1) + go func() { defer wg.Done(); _, tickErr := m.SendUpcomingPurchaseNotifications(ctx); errors <- tickErr }() + } + wg.Wait() + close(errors) + for err := range errors { + require.NoError(t, err) + } + rows := stepRowsInDB(ctx, t, store, plan.ID) + require.Len(t, rows, 1) + assert.Equal(t, root.ExecutionID, rows[0].ExecutionID) + assert.Equal(t, "notified", rows[0].Status) + assert.True(t, rows[0].ScheduledDate.Equal(next)) + require.Len(t, rows[0].Recommendations, 1) + assert.Equal(t, 20, rows[0].Recommendations[0].Count) + require.NotNil(t, rows[0].Recommendations[0].MonthlyCost) + assert.Equal(t, 200.0, *rows[0].Recommendations[0].MonthlyCost) + assert.Equal(t, 2400.0, rows[0].Recommendations[0].TotalCommitment()) + assert.Equal(t, 100.0, rows[0].EstimatedSavings) + assert.Zero(t, rows[0].TotalUpfrontCost) + eligible, err := m.executableByScheduler(ctx, &rows[0]) + require.NoError(t, err) + assert.False(t, eligible) + var suppressed int + require.NoError(t, pool.QueryRow(ctx, `SELECT COALESCE(SUM(suppressed_count),0) FROM purchase_suppressions WHERE execution_id=$1`, root.ExecutionID).Scan(&suppressed)) + assert.Equal(t, 20, suppressed) +} + +func TestManualPlanStep_OverdueEmptyRootFailsOnceWithoutEmail(t *testing.T) { + ctx := context.Background() + store, _ := rampStepStore(ctx, t) + next := time.Now().Add(-30 * 24 * time.Hour).Truncate(time.Microsecond) + plan := attachPlan(next, 100) + plan.ID, plan.Name, plan.AutoPurchase = "", "overdue-manual", false + plan.Services["azure:compute"] = config.ServiceConfig{Provider: "azure", Service: "compute", Enabled: true, Term: 1, Payment: "monthly", Coverage: 80} + plan.RampSchedule = config.RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4, StartDate: next} + require.NoError(t, store.CreatePurchasePlan(ctx, plan)) + root := &config.PurchaseExecution{ExecutionID: uuid.NewString(), PlanID: plan.ID, Status: "pending", StepNumber: 1, ScheduledDate: next} + require.NoError(t, store.SavePurchaseExecution(ctx, root)) + future := time.Now().Add(20 * 24 * time.Hour).Truncate(time.Microsecond) + plan.NextExecutionDate = &future + require.NoError(t, store.UpdatePurchasePlan(ctx, plan)) + m := &Manager{config: store} + for range 2 { + result, err := m.SendUpcomingPurchaseNotifications(ctx) + require.NoError(t, err) + assert.Zero(t, result.Notified) + } + rows := stepRowsInDB(ctx, t, store, plan.ID) + require.Len(t, rows, 1) + assert.Equal(t, "failed", rows[0].Status) + assert.True(t, rows[0].NoSubmission) + assert.Contains(t, rows[0].Error, "without recommendations") + assert.True(t, rows[0].ScheduledDate.Equal(next)) + stored, err := store.GetPurchasePlan(ctx, plan.ID) + require.NoError(t, err) + assert.Zero(t, stored.RampSchedule.CurrentStep) +} diff --git a/internal/purchase/notifications.go b/internal/purchase/notifications.go index ed04ef62..1604ec2c 100644 --- a/internal/purchase/notifications.go +++ b/internal/purchase/notifications.go @@ -37,6 +37,9 @@ func (m *Manager) SendUpcomingPurchaseNotifications(ctx context.Context) (*Notif notified := 0 for _rvc := range plans { plan := plans[_rvc] + if err := m.failDueManualStep(ctx, &plan); err != nil { + return nil, err + } if m.shouldNotifyPlan(plan, autoPurchaseEnabled) || shouldRetryOverduePlan(plan, autoPurchaseEnabled, global, time.Now()) { if m.sendPlanNotification(ctx, &plan) { notified++ @@ -52,7 +55,7 @@ func (m *Manager) SendUpcomingPurchaseNotifications(ctx context.Context) (*Notif // shouldNotifyPlan checks if a plan should trigger a notification. // autoPurchaseEnabled is the global scheduling.auto_purchase kill switch. func (m *Manager) shouldNotifyPlan(plan config.PurchasePlan, autoPurchaseEnabled bool) bool { - if !autoPurchaseEnabled || !plan.Enabled || !plan.AutoPurchase { + if !plan.Enabled || (plan.AutoPurchase && !autoPurchaseEnabled) { return false } @@ -90,7 +93,7 @@ func planGraceDays(plan *config.PurchasePlan, global *config.GlobalConfig) int { // one day past the grace window, so the first tick at or after the deadline can // write the failed row. A step that already has a row is skipped by the claim. func shouldRetryOverduePlan(plan config.PurchasePlan, autoPurchaseEnabled bool, global *config.GlobalConfig, now time.Time) bool { - if !autoPurchaseEnabled || !plan.Enabled || !plan.AutoPurchase || plan.NextExecutionDate == nil || !plan.NextExecutionDate.Before(now) { + if !plan.Enabled || (plan.AutoPurchase && !autoPurchaseEnabled) || plan.NextExecutionDate == nil || !plan.NextExecutionDate.Before(now) { return false } if plan.LastNotificationSent != nil && now.Sub(*plan.LastNotificationSent).Hours() < config.MinHoursBetweenNotifications { @@ -99,6 +102,47 @@ func shouldRetryOverduePlan(plan config.PurchasePlan, autoPurchaseEnabled bool, return now.Before(plan.NextExecutionDate.AddDate(0, 0, planGraceDays(&plan, global)+1)) } +func (m *Manager) failDueManualStep(ctx context.Context, plan *config.PurchasePlan) error { + if !plan.Enabled || plan.AutoPurchase { + return nil + } + var root *config.PurchaseExecution + if err := m.config.WithTx(ctx, func(tx pgx.Tx) error { + var err error + root, err = m.dueManualRootTx(ctx, tx, plan.ID) + return err + }); err != nil { + return fmt.Errorf("failed to inspect due manual plan step: %w", err) + } + if root == nil { + return nil + } + err := m.failExistingPlanStep(ctx, root, "scheduled plan step reached its scheduled date without recommendations; no purchase was submitted") + if errors.Is(err, errExecutionNotNotifiable) { + return nil + } + return err +} + +func (m *Manager) dueManualRootTx(ctx context.Context, tx pgx.Tx, planID string) (*config.PurchaseExecution, error) { + locked, err := m.config.LockPurchasePlanTx(ctx, tx, planID) + if err != nil { + return nil, err + } + if locked == nil || !locked.Enabled || locked.AutoPurchase { + return nil, nil + } + rows, err := m.config.ListExecutionsForPlanStepTx(ctx, tx, planID, locked.RampSchedule.CurrentStep+1) + if err != nil || len(rows) == 0 { + return nil, err + } + candidate, blocked := adoptableRootRow(rows) + if blocked == nil && len(candidate.Recommendations) == 0 && !candidate.ScheduledDate.After(time.Now()) { + return candidate, nil + } + return nil, nil +} + // errExecutionNotNotifiable marks an existing execution for the plan's date // that has already left pending/notified; every notification tick until the // plan's NextExecutionDate advances hits it, so it is skipped quietly. @@ -250,7 +294,7 @@ func adoptableRootRow(rows []config.PurchaseExecution) (*config.PurchaseExecutio // from the stale read overwriting the approval. A won CAS returns the fresh // row, which is the one the error text is written onto. func (m *Manager) failExistingPlanStep(ctx context.Context, exec *config.PurchaseExecution, reason string) error { - failed, err := m.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"pending", "notified"}, "failed", nil) + failed, err := m.config.TransitionExecutionStatusPinned(ctx, exec.ExecutionID, []string{"pending", "notified"}, "failed", nil, true) if errors.Is(err, config.ErrNotFound) || errors.Is(err, config.ErrExecutionNotInExpectedStatus) { return fmt.Errorf("%w: %s changed state before it could be failed", errExecutionNotNotifiable, exec.ExecutionID) } diff --git a/internal/purchase/notifications_test.go b/internal/purchase/notifications_test.go index ab080cdd..990a57ad 100644 --- a/internal/purchase/notifications_test.go +++ b/internal/purchase/notifications_test.go @@ -82,6 +82,7 @@ func TestManager_SendUpcomingPurchaseNotifications_NotAutoPurchase(t *testing.T) } mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) + expectStepRows(mockStore, &plans[0]) manager := &Manager{ config: mockStore, @@ -278,7 +279,7 @@ func TestManager_GetOrCreateExecution_ExistingBareRowIsFailedNotRotated(t *testi expectStepRows(mockStore, plan, *bare) failed := *bare failed.Status = "failed" - mockStore.On("TransitionExecutionStatus", ctx, "bare-exec-id", []string{"pending", "notified"}, "failed", (*string)(nil)).Return(&failed, nil) + mockStore.On("TransitionExecutionStatusPinned", ctx, "bare-exec-id", []string{"pending", "notified"}, "failed", (*string)(nil), true).Return(&failed, nil) mockStore.On("SavePurchaseExecution", ctx, &failed).Return(nil) manager := &Manager{config: mockStore, resolver: &fakeResolver{err: planstep.ErrNoMatch}} @@ -307,7 +308,7 @@ func TestManager_GetOrCreateExecution_ExistingBareRowApprovedMeanwhileIsNotOverw ExecutionID: "bare-exec-id", PlanID: "plan-123", Status: "pending", StepNumber: 1, ScheduledDate: nextExec, } expectStepRows(mockStore, plan, *stale) - mockStore.On("TransitionExecutionStatus", ctx, "bare-exec-id", []string{"pending", "notified"}, "failed", (*string)(nil)). + mockStore.On("TransitionExecutionStatusPinned", ctx, "bare-exec-id", []string{"pending", "notified"}, "failed", (*string)(nil), true). Return(nil, config.ErrExecutionNotInExpectedStatus) // No SavePurchaseExecution expectation: an upsert here would clobber the approval. diff --git a/internal/purchase/plan_step_atomic_test.go b/internal/purchase/plan_step_atomic_test.go index 3f5469b0..6a24e00d 100644 --- a/internal/purchase/plan_step_atomic_test.go +++ b/internal/purchase/plan_step_atomic_test.go @@ -17,7 +17,7 @@ import ( func stepTestPlan() *config.PurchasePlan { next := time.Now().Add(24 * time.Hour) - return &config.PurchasePlan{ID: "plan-s", RampSchedule: config.RampSchedule{CurrentStep: 1}, NextExecutionDate: &next} + return &config.PurchasePlan{ID: "plan-s", Enabled: true, AutoPurchase: true, RampSchedule: config.RampSchedule{CurrentStep: 1}, NextExecutionDate: &next} } func stepRow(status string, mutate func(*config.PurchaseExecution)) config.PurchaseExecution { diff --git a/internal/purchase/plan_step_attach.go b/internal/purchase/plan_step_attach.go index 5d2a564d..743c5ef2 100644 --- a/internal/purchase/plan_step_attach.go +++ b/internal/purchase/plan_step_attach.go @@ -104,6 +104,9 @@ func (m *Manager) claimFromRows(ctx context.Context, tx pgx.Tx, locked *config.P } return m.attachStep(ctx, tx, locked, claim, root) } + if !locked.AutoPurchase { + return stepClaim{}, fmt.Errorf("%w: manual plan has no scheduled root", errPlanStepSkipped) + } return m.attachStep(ctx, tx, locked, claim, nil) } @@ -127,7 +130,7 @@ func planProviders(plan *config.PurchasePlan) []string { // compared at the microsecond precision PostgreSQL stores, so an in-memory plan // with nanoseconds (Linux clocks) is not mistaken for a changed one. func planUnchanged(listed, locked *config.PurchasePlan, providers []string) bool { - return locked.Enabled && locked.AutoPurchase && locked.NextExecutionDate != nil && listed.NextExecutionDate != nil && + return locked.Enabled && locked.AutoPurchase == listed.AutoPurchase && locked.NextExecutionDate != nil && listed.NextExecutionDate != nil && locked.NextExecutionDate.Truncate(time.Microsecond).Equal(listed.NextExecutionDate.Truncate(time.Microsecond)) && strings.Join(planProviders(locked), ",") == strings.Join(providers, ",") } @@ -167,8 +170,14 @@ func (m *Manager) attachStep(ctx context.Context, tx pgx.Tx, locked *config.Purc } recs := res.Recs sort.Slice(recs, func(i, j int) bool { return recs[i].ID < recs[j].ID }) - if capErr := CheckStepCommitmentCap(locked, recs); capErr != nil { - return m.failStep(ctx, tx, locked, claim, root, fmt.Sprintf("%v (recommendations: %s)", capErr, recIDs(recs))) + return m.attachWithinCap(ctx, tx, locked, claim, root, recs) +} + +func (m *Manager) attachWithinCap(ctx context.Context, tx pgx.Tx, locked *config.PurchasePlan, claim stepClaim, root *config.PurchaseExecution, recs []config.RecommendationRecord) (stepClaim, error) { + if locked.AutoPurchase || locked.MaxStepCommitment != nil { + if capErr := CheckStepCommitmentCap(locked, recs); capErr != nil { + return m.failStep(ctx, tx, locked, claim, root, fmt.Sprintf("%v (recommendations: %s)", capErr, recIDs(recs))) + } } return m.writeAttached(ctx, tx, locked, claim, root, recs) } diff --git a/internal/purchase/plan_step_attach_test.go b/internal/purchase/plan_step_attach_test.go index 1f6b7f59..87604146 100644 --- a/internal/purchase/plan_step_attach_test.go +++ b/internal/purchase/plan_step_attach_test.go @@ -259,3 +259,105 @@ func TestAttach_UnsupportedPlanFailsTheStepVisibly(t *testing.T) { assert.Equal(t, "failed", saved[0].Status) assert.Contains(t, saved[0].Error, "multi-step ramps do not support Savings Plans") } + +func TestAttach_ManualPlanRequiresExplicitRootAndHonorsOptionalCap(t *testing.T) { + for _, tc := range []struct { + name string + cap *float64 + root bool + wantAttach bool + }{ + {"no implicit row", nil, false, false}, + {"manual without cap", nil, true, true}, + {"manual exact cap", f64(240), true, true}, + {"manual over cap", f64(239), true, false}, + } { + t.Run(tc.name, func(t *testing.T) { + plan := attachPlan(time.Now().Add(84*time.Hour), 1e6) + plan.AutoPurchase, plan.MaxStepCommitment = false, tc.cap + root := config.PurchaseExecution{ExecutionID: "manual-root", PlanID: plan.ID, Status: "pending", StepNumber: 1, ScheduledDate: *plan.NextExecutionDate} + var rows []config.PurchaseExecution + if tc.root { + rows = append(rows, root) + } + res := &fakeResolver{recs: attachRecs()} + m, store := attachManager(t, plan, plan, res, rows...) + for _, expected := range store.ExpectedCalls { + if expected.Method == "GetGlobalConfig" { + expected.Maybe() + } + } + store.On("SetExecutionRecommendationsIfEmptyTx", mock.Anything, mock.Anything, root.ExecutionID, mock.Anything, mock.Anything, mock.Anything).Return(true, nil).Maybe() + store.On("CreateSuppressionTx", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + failed := root + failed.Status = "failed" + store.On("TransitionExecutionStatusPinned", mock.Anything, root.ExecutionID, []string{"pending", "notified"}, "failed", (*string)(nil), true).Return(&failed, nil).Maybe() + store.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil).Maybe() + exec, _, _, err := m.getOrCreateExecution(context.Background(), plan) + if tc.wantAttach { + require.NoError(t, err) + require.Len(t, exec.Recommendations, 2) + assert.Equal(t, root.ExecutionID, exec.ExecutionID) + assert.Equal(t, root.ScheduledDate, exec.ScheduledDate) + } else { + require.ErrorIs(t, err, errExecutionNotNotifiable) + if !tc.root { + assert.Zero(t, res.calls.Load()) + } else { + assert.Contains(t, err.Error(), "exceeds max_step_commitment") + } + } + store.AssertExpectations(t) + }) + } +} + +func TestManualDueStepCleanupWithoutEmailAfterLongDowntime(t *testing.T) { + for _, tc := range []struct { + name string + future bool + lostClaim bool + futurePlan bool + }{ + {"due empty root", false, false, false}, + {"attach wins race", false, true, false}, + {"future root", true, false, false}, + {"due root with future plan date", false, false, true}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + plan := attachPlan(time.Now().Add(-20*24*time.Hour), 1) + plan.AutoPurchase = false + root := config.PurchaseExecution{ExecutionID: "due-root", PlanID: plan.ID, Status: "pending", StepNumber: 1, ScheduledDate: *plan.NextExecutionDate} + if tc.futurePlan { + next := time.Now().Add(20 * 24 * time.Hour) + plan.NextExecutionDate = &next + } + if tc.future { + root.ScheduledDate = time.Now().Add(time.Hour) + } + m, store := attachManager(t, plan, plan, &fakeResolver{}, root) + store.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return([]config.PurchasePlan{*plan}, nil) + failed := root + failed.Status = "failed" + if !tc.future { + if tc.lostClaim { + store.On("TransitionExecutionStatusPinned", ctx, root.ExecutionID, []string{"pending", "notified"}, "failed", (*string)(nil), true).Return(nil, config.ErrExecutionNotInExpectedStatus).Once() + } else { + store.On("TransitionExecutionStatusPinned", ctx, root.ExecutionID, []string{"pending", "notified"}, "failed", (*string)(nil), true).Return(&failed, nil).Once() + store.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(row *config.PurchaseExecution) bool { + return row.ExecutionID == root.ExecutionID && row.NoSubmission && row.Status == "failed" && len(row.Recommendations) == 0 && row.ScheduledDate.Equal(root.ScheduledDate) + })).Return(nil).Once() + } + } + result, err := m.SendUpcomingPurchaseNotifications(ctx) + require.NoError(t, err) + assert.Zero(t, result.Notified) + assert.Zero(t, plan.RampSchedule.CurrentStep) + if tc.future || tc.lostClaim { + store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + } + store.AssertExpectations(t) + }) + } +} diff --git a/internal/purchase/step_rows_test.go b/internal/purchase/step_rows_test.go index c4f33d26..5e228882 100644 --- a/internal/purchase/step_rows_test.go +++ b/internal/purchase/step_rows_test.go @@ -8,9 +8,8 @@ import ( // expectStepRows registers the plan lock and the per-step row listing that // getOrCreateExecution reads under the ramp lock. func expectStepRows(m *MockConfigStore, plan *config.PurchasePlan, rows ...config.PurchaseExecution) { - // The tick only reaches the lock for an enabled auto-purchase plan. locked := *plan - locked.Enabled, locked.AutoPurchase = true, true + locked.Enabled = true m.On("LockPurchasePlanTx", mock.Anything, mock.Anything, plan.ID).Return(&locked, nil) m.On("ListExecutionsForPlanStepTx", mock.Anything, mock.Anything, plan.ID, plan.RampSchedule.CurrentStep+1).Return(rows, nil) } From 5ea04b251a550c4fca127b4e2600241e933dca14 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 21:07:39 +0200 Subject: [PATCH 2/3] fix(ci): bound integration package runtime at twenty minutes --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 41fccbf3..3d55fae5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -430,7 +430,7 @@ jobs: continue fi echo "==> integration tests in $mod" - if ! (cd "$mod" && go test -v -race -tags=integration \ + if ! (cd "$mod" && go test -v -race -tags=integration -timeout=20m \ -coverprofile="$RUNNER_TEMP/coverage-integration-${tag}.out" \ ./...) 2>&1 | tee "$log"; then echo "::error::integration tests failed in $mod" From 75ce0fc81a318b5f82940922d3b48e815ea979f8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 23:54:08 +0200 Subject: [PATCH 3/3] fix(purchase): isolate manual cleanup failures per plan (#871) --- internal/purchase/notifications.go | 3 +- internal/purchase/notifications_test.go | 37 +++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/internal/purchase/notifications.go b/internal/purchase/notifications.go index 1604ec2c..83a7935f 100644 --- a/internal/purchase/notifications.go +++ b/internal/purchase/notifications.go @@ -38,7 +38,8 @@ func (m *Manager) SendUpcomingPurchaseNotifications(ctx context.Context) (*Notif for _rvc := range plans { plan := plans[_rvc] if err := m.failDueManualStep(ctx, &plan); err != nil { - return nil, err + logging.Errorf("Failed to inspect due manual plan step for plan %s: %v", plan.ID, err) + continue } if m.shouldNotifyPlan(plan, autoPurchaseEnabled) || shouldRetryOverduePlan(plan, autoPurchaseEnabled, global, time.Now()) { if m.sendPlanNotification(ctx, &plan) { diff --git a/internal/purchase/notifications_test.go b/internal/purchase/notifications_test.go index 990a57ad..8475ac5d 100644 --- a/internal/purchase/notifications_test.go +++ b/internal/purchase/notifications_test.go @@ -9,6 +9,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/email" "github.com/LeanerCloud/cloud-commitments-platform/internal/planstep" + "github.com/jackc/pgx/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -539,6 +540,42 @@ func TestManager_SendUpcomingPurchaseNotifications_WithNotification(t *testing.T mockEmail.AssertExpectations(t) } +func TestManager_SendUpcomingPurchaseNotifications_ManualCleanupErrorDoesNotBlockLaterPlan(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + sender := new(MockEmailSender) + nextDate := time.Now().Add(3 * 24 * time.Hour) + plans := []config.PurchasePlan{ + {ID: "manual-failed", Enabled: true, NotificationDaysBefore: 7, NextExecutionDate: &nextDate}, + {ID: "automatic-next", Enabled: true, AutoPurchase: true, NotificationDaysBefore: 7, NextExecutionDate: &nextDate}, + } + address := "fixture@example.invalid" + store.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) + store.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &address, AutoPurchaseEnabled: true}, nil) + store.On("WithTx", ctx, mock.Anything).Return(errors.New("manual cleanup transaction failed")).Once() + store.On("WithTx", ctx, mock.Anything).Return(nil).Run(func(args mock.Arguments) { + require.NoError(t, args.Get(1).(func(pgx.Tx) error)(nil)) + }).Once() + expectStepRows(store, &plans[1], config.PurchaseExecution{ + ExecutionID: "automatic-root", PlanID: plans[1].ID, Status: "pending", StepNumber: 1, + ScheduledDate: nextDate, Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Count: 1}}, + }) + store.On("RotatePendingApprovalToken", ctx, "automatic-root", mock.Anything, mock.Anything).Return(true, nil).Once() + store.On("StampPlanNotificationSent", ctx, plans[1].ID, mock.AnythingOfType("time.Time")).Return(nil).Once() + sender.On("SendScheduledPurchaseNotification", ctx, mock.MatchedBy(func(data email.NotificationData) bool { + return data.PlanID == plans[1].ID + })).Return(nil).Once() + + manager := &Manager{config: store, email: sender, dashboardURL: "https://dashboard.example.invalid"} + result, err := manager.SendUpcomingPurchaseNotifications(ctx) + require.NoError(t, err) + require.Equal(t, 1, result.Notified) + store.AssertNotCalled(t, "LockPurchasePlanTx", mock.Anything, mock.Anything, plans[0].ID) + store.AssertNotCalled(t, "StampPlanNotificationSent", mock.Anything, plans[0].ID, mock.Anything) + store.AssertExpectations(t) + sender.AssertExpectations(t) +} + func TestManager_SendUpcomingPurchaseNotifications_TooFarAway(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore)