diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 70e1f2380..5a6687bfd 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -708,11 +708,19 @@ func (m *Manager) safeFail(ctx context.Context, exec *config.PurchaseExecution) } return false, fmt.Errorf("failed to transition stranded execution %s to failed: %w", exec.ExecutionID, txErr) } - // This sweep won the CAS, so it is the one writer that notifies (#1195). - defer m.notifyExecutionFailure(ctx, updated) + // This sweep won the CAS, so it is the one writer that notifies (#1195), + // unless the guarded stamp below finds the row already recovered: then the + // in-memory failed row is stale and a failure email would be false. + notify := true + defer func() { + if notify { + m.notifyExecutionFailure(ctx, updated) + } + }() updated.Error = strandedRecoveryError(exec) if saveErr := m.config.SaveExecutionIfStatus(ctx, updated, []string{config.StatusFailed}); saveErr != nil { + notify = !errors.Is(saveErr, config.ErrExecutionNotInExpectedStatus) // Guarded on "failed" (issue #835): the stamp lands only while the row // is still in the status this CAS set, never over a later writer's. // The atomic flip to "failed" already landed via TransitionExecutionStatus; diff --git a/internal/purchase/post_cas_guarded_save_integration_test.go b/internal/purchase/post_cas_guarded_save_integration_test.go index 9e907a94d..996c44568 100644 --- a/internal/purchase/post_cas_guarded_save_integration_test.go +++ b/internal/purchase/post_cas_guarded_save_integration_test.go @@ -19,6 +19,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/jackc/pgx/v5" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -91,19 +92,13 @@ func TestPostCASSave_ConcurrentStatusChangeSurvives(t *testing.T) { require.NoError(t, f.store.RecoverReapedExecution(ctx, late)) }) - // safeFail's flip to failed lands before the late recovery, so it alerts - // as a terminal-failure writer (#1245). The rows below are what this test - // pins; the alert's content is pinned in failure_notify*_test.go. - sender, ok := f.manager.email.(*MockEmailSender) - require.True(t, ok) - failedMail := &failedEmailRecorder{} - failedMail.expect(sender) - + // The late recovery completes the row before the guarded stamp, so the + // failed outcome is stale and no failure email may go out. counted, err := f.manager.safeFail(ctx, rowStatusCopy(row)) require.NoError(t, err) - assert.LessOrEqual(t, len(failedMail.all()), 1, "the race must not alert twice for one row") assert.True(t, counted, "the flip to failed landed, so the sweep still counts it") + f.email.AssertNumberOfCalls(t, "SendPurchaseFailedNotification", 0) got := rowStatus(ctx, t, f, row.ExecutionID) assert.Equal(t, config.StatusCompleted, got.Status, "the recovered purchase must not be reverted to failed") assert.NotContains(t, got.Error, "failed by the recovery sweep") @@ -111,6 +106,20 @@ func TestPostCASSave_ConcurrentStatusChangeSurvives(t *testing.T) { assert.True(t, got.Recommendations[0].Purchased) }) + // Without the interleaved recovery the same flip stays failed and is + // announced exactly once (#1195). + t.Run("safeFail flip without a recovery notifies once", func(t *testing.T) { + row := claimRaceRow(ctx, t, f, "approved") + f.email.On("SendPurchaseFailedNotification", mock.Anything, mock.Anything).Return(nil).Once() + + counted, err := f.manager.safeFail(ctx, rowStatusCopy(row)) + + require.NoError(t, err) + assert.True(t, counted) + assert.Equal(t, config.StatusFailed, rowStatus(ctx, t, f, row.ExecutionID).Status) + f.email.AssertNumberOfCalls(t, "SendPurchaseFailedNotification", 1) + }) + // A session revoke (revokeViaSession's CAS) lands right after the terminal // save; the revocation-token rotation and the ramp-refusal note then write // the caller's in-memory "completed" row. Neither may erase the request. diff --git a/internal/purchase/ramp_step_progress_integration_test.go b/internal/purchase/ramp_step_progress_integration_test.go index 538aed696..59031e1ee 100644 --- a/internal/purchase/ramp_step_progress_integration_test.go +++ b/internal/purchase/ramp_step_progress_integration_test.go @@ -55,6 +55,7 @@ func rampStepStore(ctx context.Context, t *testing.T) (*config.PostgresStore, *p type rampStepFixture struct { store *config.PostgresStore manager *Manager + email *MockEmailSender plan *config.PurchasePlan accounts []config.CloudAccount @@ -133,6 +134,7 @@ func newRampStepFixture(ctx context.Context, t *testing.T) *rampStepFixture { f := &rampStepFixture{store: store, plan: plan, accounts: accounts} mockEmail := new(MockEmailSender) + f.email = mockEmail mockFactory := new(MockProviderFactory) mockProviderInst := new(MockProvider) mockServiceClient := new(MockServiceClient)