From ad84d14296d7c25bda605c01550a3fdafd19a204 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 5 Oct 2026 08:52:48 +0200 Subject: [PATCH 1/2] fix(api): map run-now failures by cause and stamp its audit fields Every RunPlannedPurchaseNow error used to come back as a 409 "cannot be started", including a provider failure or a partial completion where money had already moved. The handler now maps a 4-eyes refusal to 403, a lost claim or missing row to 409, a store error before the claim to 500, an unsaved final status to 500, and an execution failure to 502 carrying the row's finalized status (failed / partially_completed). To tell "never started" from "ran and failed", RunPlannedPurchaseNow now returns the claimed row (nil when the CAS was never won), and every 4-eyes refusal wraps the new purchase.ErrFourEyesDenied sentinel. The success response reports that row's status instead of a hardcoded "completed", and the executed email uses the post-run row instead of the pre-run snapshot. Run-now also stamps executed_at, executed_by_user_id and pre_approval_skip_reason = "run-now" like direct-execute does, but after the CAS is won and in the same save as approved_by: a pre-claim save of the stale row could revert a concurrent scheduler claim. Refs #386 --- internal/api/execution_scope_test.go | 2 +- internal/api/handler_purchases.go | 26 +++- ...dler_purchases_run_now_integration_test.go | 144 ++++++++++++++++++ internal/api/handler_purchases_test.go | 41 ++++- internal/api/handler_test.go | 2 +- internal/api/mocks_test.go | 5 +- internal/api/types.go | 2 +- internal/config/types.go | 7 +- internal/purchase/approvals.go | 52 +++++-- internal/purchase/approvals_test.go | 7 +- internal/server/interfaces.go | 2 +- internal/testutil/mocks.go | 6 +- 12 files changed, 262 insertions(+), 34 deletions(-) create mode 100644 internal/api/handler_purchases_run_now_integration_test.go diff --git a/internal/api/execution_scope_test.go b/internal/api/execution_scope_test.go index 2944998a..f693d3aa 100644 --- a/internal/api/execution_scope_test.go +++ b/internal/api/execution_scope_test.go @@ -212,7 +212,7 @@ func newScopeTestHandler(t *testing.T, exec *config.PurchaseExecution, scope []s store.On("CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(false, "", errScopeTestReached).Maybe() store.On("CancelScheduledExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(false, "", errScopeTestReached).Maybe() mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return("", errScopeTestReached).Maybe() - mockPurchase.On("RunPlannedPurchaseNow", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return("", errScopeTestReached).Maybe() + mockPurchase.On("RunPlannedPurchaseNow", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, "", errScopeTestReached).Maybe() mockPurchase.On("CancelExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() return &Handler{auth: mockAuth, config: store, purchase: mockPurchase} diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 13b14b51..57621a99 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -345,23 +345,41 @@ func (h *Handler) runPlannedPurchase(ctx context.Context, req *events.LambdaFunc return nil, constraintErr } - revocationToken, runErr := h.purchase.RunPlannedPurchaseNow(ctx, executionID, fourEyesActorIdentity(session), resolveCreatorUserID(session)) + final, revocationToken, runErr := h.purchase.RunPlannedPurchaseNow(ctx, executionID, fourEyesActorIdentity(session), resolveCreatorUserID(session)) if runErr != nil { - return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be started: %v", executionID, runErr)) + return nil, runNowError(executionID, final, runErr) } // The shared execute funnel rotated the row's token into a revocation // token whose raw value exists only here (issue #103); email it like the // session-approve path does. Best-effort: the purchase already committed. - h.sendPurchaseExecutedEmail(ctx, req, execution, revocationToken, session.Email) + h.sendPurchaseExecutedEmail(ctx, req, final, revocationToken, session.Email) return map[string]any{ "execution_id": executionID, - "status": "completed", + "status": final.Status, "message": "Purchase executed", }, nil } +// runNowError maps a RunPlannedPurchaseNow failure to its HTTP error. A non-nil +// final means the purchase ran and money may have moved, so it is never a 409. +func runNowError(executionID string, final *config.PurchaseExecution, err error) error { + switch { + case errors.Is(err, purchase.ErrFourEyesDenied): + return NewClientError(403, fmt.Sprintf("execution %s cannot be started: %v", executionID, err)) + case errors.Is(err, config.ErrExecutionNotInExpectedStatus), errors.Is(err, config.ErrNotFound): + return NewClientError(409, fmt.Sprintf("execution %s cannot be started: %v", executionID, err)) + case final == nil: + return fmt.Errorf("execution %s could not be started: %w", executionID, err) + case errors.Is(err, config.ErrAuditLoss): + return NewClientError(500, fmt.Sprintf("execution %s ran but its final status could not be saved: %v", executionID, err)) + default: + return NewClientErrorWithDetails(502, fmt.Sprintf("execution %s failed: %v", executionID, err), + map[string]any{"execution_id": executionID, "status": final.Status}) + } +} + // requireDeleteOrCancelPurchasePermission validates the request session and // returns it when the caller holds any of: delete:purchases (management), // update-any:purchases (full-scope management), cancel-any:purchases diff --git a/internal/api/handler_purchases_run_now_integration_test.go b/internal/api/handler_purchases_run_now_integration_test.go new file mode 100644 index 00000000..7d4cd568 --- /dev/null +++ b/internal/api/handler_purchases_run_now_integration_test.go @@ -0,0 +1,144 @@ +//go:build integration + +package api + +import ( + "context" + "errors" + "sync/atomic" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// runNowService buys every recommendation except the call numbered failOn. +type runNowService struct { + fanoutServiceClient + calls atomic.Int32 + failOn int32 +} + +func (s *runNowService) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { + if s.calls.Add(1) == s.failOn { + return common.PurchaseResult{}, errors.New("provider rejected the purchase") + } + return s.fanoutServiceClient.PurchaseCommitment(ctx, rec, opts) +} + +type runNowProvider struct { + fanoutProvider + service *runNowService +} + +func (p *runNowProvider) GetServiceClient(context.Context, common.ServiceType, string) (provider.ServiceClient, error) { + return p.service, nil +} + +type runNowFactory struct{ provider *runNowProvider } + +func (f runNowFactory) CreateAndValidateProvider(context.Context, string, *provider.ProviderConfig) (provider.Provider, error) { + return f.provider, nil +} + +// newRunNowFixture is the pause-claim fixture (real Postgres store, pending +// row on an auto-purchase plan) with the handler wired to a real manager whose +// provider fails on purchase call failOn (0 = never), run by the row's creator +// holding execute:purchases. +func newRunNowFixture(t *testing.T, failOn int32, recs []config.RecommendationRecord) (*pauseClaimFixture, *runNowService) { + t.Helper() + f := newPauseClaimFixture(t) + f.execution.Recommendations = recs + require.NoError(t, f.store.SavePurchaseExecution(f.ctx, f.execution)) + service := &runNowService{failOn: failOn} + mockAuth := new(MockAuthService) + session := &Session{UserID: *f.execution.CreatedByUserID, Email: "creator@example.com"} + mockAuth.On("ValidateSession", mock.Anything, "admin-token").Return(session, nil).Maybe() + mockAuth.grantAdminPurchaser() + f.handler.auth = mockAuth + f.handler.purchase = purchase.NewManager(purchase.ManagerConfig{ + ConfigStore: f.store, EmailSender: &stubEmailNotifier{}, CredentialStore: &fanoutCredStore{}, + ProviderFactory: runNowFactory{provider: &runNowProvider{service: service}}, + }) + return f, service +} + +func (f *pauseClaimFixture) runNow() (any, error) { + return f.handler.runPlannedPurchase(f.ctx, f.request, f.execution.ExecutionID) +} + +func twoRunNowRecs() []config.RecommendationRecord { + recs := append(fanoutRecs(), fanoutRecs()...) + recs[1].ResourceType = "m5.xlarge" + return recs +} + +func TestRunNow_StampsExecutedAuditFieldsAndReturnsRowStatus(t *testing.T) { + f, service := newRunNowFixture(t, 0, fanoutRecs()) + before := time.Now() + result, err := f.runNow() + require.NoError(t, err) + + row, err := f.store.GetExecutionByID(f.ctx, f.execution.ExecutionID) + require.NoError(t, err) + assert.Equal(t, "completed", row.Status) + assert.Equal(t, row.Status, result.(map[string]any)["status"]) + assert.EqualValues(t, 1, service.calls.Load()) + require.NotNil(t, row.ExecutedAt, "run-now must stamp executed_at") + assert.WithinRange(t, *row.ExecutedAt, before.Add(-time.Second), time.Now()) + require.NotNil(t, row.ExecutedByUserID, "run-now must stamp executed_by_user_id") + assert.Equal(t, *f.execution.CreatedByUserID, *row.ExecutedByUserID) + require.NotNil(t, row.PreApprovalSkipReason) + assert.Equal(t, "run-now", *row.PreApprovalSkipReason) +} + +func TestRunNow_PartialFailureIsServerErrorWithRowStatus(t *testing.T) { + f, service := newRunNowFixture(t, 2, twoRunNowRecs()) + result, err := f.runNow() + assert.Nil(t, result) + + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client-facing error, got %v", err) + assert.Equal(t, 502, ce.code, "money moved: this is an execution failure, not a 409 conflict") + row, getErr := f.store.GetExecutionByID(f.ctx, f.execution.ExecutionID) + require.NoError(t, getErr) + assert.Equal(t, "partially_completed", row.Status) + assert.Equal(t, row.Status, ce.Details()["status"]) + assert.EqualValues(t, 2, service.calls.Load()) + assert.NotNil(t, row.ExecutedAt) +} + +func TestRunNow_FourEyesDenialIsForbiddenAndLeavesRowUntouched(t *testing.T) { + f, service := newRunNowFixture(t, 0, fanoutRecs()) + require.NoError(t, f.store.SaveGlobalConfig(f.ctx, &config.GlobalConfig{RequireDifferentApprover: true})) + + _, err := f.runNow() + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client-facing error, got %v", err) + assert.Equal(t, 403, ce.code) + row, getErr := f.store.GetExecutionByID(f.ctx, f.execution.ExecutionID) + require.NoError(t, getErr) + assert.Equal(t, "pending", row.Status) + assert.Nil(t, row.ExecutedAt) + assert.Zero(t, service.calls.Load()) +} + +func TestRunNow_LostClaimIsConflict(t *testing.T) { + f, service := newRunNowFixture(t, 0, fanoutRecs()) + f.execution.Status = "running" + require.NoError(t, f.store.SavePurchaseExecution(f.ctx, f.execution)) + + _, err := f.runNow() + assertPauseConflict(t, err) + row, getErr := f.store.GetExecutionByID(f.ctx, f.execution.ExecutionID) + require.NoError(t, getErr) + assert.Equal(t, "running", row.Status) + assert.Nil(t, row.ExecutedAt) + assert.Zero(t, service.calls.Load()) +} diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 62735e63..f3df3a8e 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1592,7 +1592,11 @@ func TestHandler_runPlannedPurchase(t *testing.T) { // CAS-guarded funnel ApproveAndExecute uses (issue #218) rather than a // bare TransitionExecutionStatus flip to "running" that no executor // consumes. - mockPurchase.On("RunPlannedPurchaseNow", ctx, "11111111-1111-1111-1111-111111111111", "admin@example.com", mock.Anything).Return("raw-revocation-token", nil) + mockPurchase.On("RunPlannedPurchaseNow", ctx, "11111111-1111-1111-1111-111111111111", "admin@example.com", mock.Anything).Return(&config.PurchaseExecution{ + ExecutionID: "11111111-1111-1111-1111-111111111111", + Status: "completed", + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 100}}, + }, "raw-revocation-token", nil) notify := "notify@example.com" mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: ¬ify}, nil) @@ -2183,7 +2187,7 @@ func TestHandler_runPlannedPurchase_NilExecution(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() mockPurchase.On("RunPlannedPurchaseNow", ctx, "99999999-9999-9999-9999-999999999999", "admin@example.com", mock.Anything). - Return("", fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) + Return(nil, "", fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) handler := &Handler{purchase: mockPurchase, config: mockStore, auth: mockAuth} @@ -2197,6 +2201,39 @@ func TestHandler_runPlannedPurchase_NilExecution(t *testing.T) { assert.Nil(t, result) } +func TestRunNowError(t *testing.T) { + ran := &config.PurchaseExecution{Status: "failed"} + cases := []struct { + name string + final *config.PurchaseExecution + err error + code int // 0: not a client error, so the router answers a generic 500 + }{ + {"four-eyes denial", nil, fmt.Errorf("%w: same approver", purchase.ErrFourEyesDenied), 403}, + {"lost claim", nil, fmt.Errorf("approve: %w", config.ErrExecutionNotInExpectedStatus), 409}, + {"row gone", nil, fmt.Errorf("approve: %w", config.ErrNotFound), 409}, + {"store error before claim", nil, errors.New("connection reset"), 0}, + {"final status not saved", ran, fmt.Errorf("%w: boom", config.ErrAuditLoss), 500}, + {"execution failed", ran, errors.New("provider rejected"), 502}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := runNowError("exec-1", tc.final, tc.err) + ce, ok := IsClientError(err) + if tc.code == 0 { + assert.False(t, ok) + assert.ErrorIs(t, err, tc.err) + return + } + require.True(t, ok) + assert.Equal(t, tc.code, ce.code) + if tc.code == 502 { + assert.Equal(t, "failed", ce.Details()["status"]) + } + }) + } +} + func TestHandler_deletePlannedPurchase_NilExecution(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 6e3678cb..ea4be066 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -1270,7 +1270,7 @@ func TestHandler_HandleRequest_RunPlannedPurchase(t *testing.T) { // resolves to the stateless admin-api-key principal rather than the // bearer-token session, so the actor identity fourEyesActorIdentity // derives is the sentinel, not the session's email. - mockPurchase.On("RunPlannedPurchaseNow", mock.Anything, "11111111-1111-1111-1111-111111111111", "admin-api-key", mock.Anything).Return("", nil) + mockPurchase.On("RunPlannedPurchaseNow", mock.Anything, "11111111-1111-1111-1111-111111111111", "admin-api-key", mock.Anything).Return(&config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "completed"}, "", nil) handler := &Handler{purchase: mockPurchase, config: mockStore, auth: mockAuth, corsAllowedOrigin: "*", apiKey: "test-key"} diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index e6eb1c29..228fdcba 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -58,9 +58,10 @@ func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, act return args.String(0), args.Error(1) } -func (m *MockPurchaseManager) RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string) (string, error) { +func (m *MockPurchaseManager) RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { args := m.Called(ctx, execID, actor, transitionedBy) - return args.String(0), args.Error(1) + exec, _ := args.Get(0).(*config.PurchaseExecution) + return exec, args.String(1), args.Error(2) } func (m *MockPurchaseManager) CancelExecution(ctx context.Context, execID, token, actor string) error { diff --git a/internal/api/types.go b/internal/api/types.go index 7dfc9e4e..55653f4c 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -146,7 +146,7 @@ type PurchaseManagerInterface interface { // execute immediately (the "Run now" button), sharing ApproveAndExecute's // 4-eyes-gated, CAS-guarded funnel instead of a bare status flip that // nothing else consumes (issue #218). - RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string) (string, error) + RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) CancelExecution(ctx context.Context, execID, token, actor string) error } diff --git a/internal/config/types.go b/internal/config/types.go index d5e1fab7..ecb53209 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -355,17 +355,18 @@ type PurchaseExecution struct { // always carry a non-nil value. ApprovalTokenExpiresAt *time.Time `json:"approval_token_expires_at,omitempty" dynamodbav:"approval_token_expires_at,omitempty"` // ExecutedByUserID is the UUID of the session user who triggered a - // direct-execute (issue #289, execute-any/execute-own). NULL on rows + // direct-execute (issue #289, execute-any/execute-own) or a run-now. NULL on rows // that went through the normal approval flow. Non-null signals the // approval step was intentionally skipped by an authorized operator. // Migration 000058 adds the column. ExecutedByUserID *string `json:"executed_by_user_id,omitempty" dynamodbav:"executed_by_user_id,omitempty"` - // ExecutedAt is the UTC timestamp when the direct-execute path fired. + // ExecutedAt is the UTC timestamp when the direct-execute or run-now path fired. // NULL for rows on the normal approval flow. Migration 000058. ExecutedAt *time.Time `json:"executed_at,omitempty" dynamodbav:"executed_at,omitempty"` // PreApprovalSkipReason is a human-readable token describing why the // approval step was skipped. For direct-execute rows it is the literal - // string "direct-execute permission". NULL on every normal-flow row. + // string "direct-execute permission", for run-now rows "run-now". NULL on + // every normal-flow row. // Migration 000058. PreApprovalSkipReason *string `json:"pre_approval_skip_reason,omitempty" dynamodbav:"pre_approval_skip_reason,omitempty"` // IdempotencyKey is the stable lineage anchor the per-rec provider diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index 5e8b87e8..ab49bc10 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -13,6 +13,14 @@ import ( "github.com/jackc/pgx/v5" ) +// ErrFourEyesDenied wraps every refusal by the 4-eyes approval policy, as +// opposed to a failure to evaluate it (config or store errors). +var ErrFourEyesDenied = errors.New("approval declined") + +// runNowSkipReason is stamped onto PreApprovalSkipReason when an operator's +// "Run now" stands in for the approval and the scheduled wait. +const runNowSkipReason = "run-now" + // ApproveExecution is the token-authenticated approve entry point used by // the legacy email-link flow and the SQS approve worker. After validating // the approval token it hands off to ApproveAndExecute, which performs the @@ -239,7 +247,7 @@ func (m *Manager) loadExecutionForFourEyes(ctx context.Context, executionID stri func (m *Manager) checkDifferentApprover(ctx context.Context, executionID string, execution *config.PurchaseExecution, actorEmail string, actorUserID *string) error { if execution.CreatedByUserID == nil { logging.Warnf("purchase[%s]: 4-eyes mode on; NULL creator (legacy row), denying", executionID) - return fmt.Errorf("approval declined: this execution predates the dual-control feature and has no recorded creator; an admin must disable 4-eyes mode to approve") + return fmt.Errorf("%w: this execution predates the dual-control feature and has no recorded creator; an admin must disable 4-eyes mode to approve", ErrFourEyesDenied) } // Tier 1: authoritative UUID comparison when the caller identified the @@ -249,7 +257,7 @@ func (m *Manager) checkDifferentApprover(ctx context.Context, executionID string if *actorUserID == *execution.CreatedByUserID { logging.Warnf("purchase[%s]: 4-eyes mode on; creator %s attempted self-approval (actor UUID match), denied", executionID, *execution.CreatedByUserID) - return fmt.Errorf("approval declined: 4-eyes mode requires a different approver than the requester") + return fmt.Errorf("%w: 4-eyes mode requires a different approver than the requester", ErrFourEyesDenied) } return nil } @@ -259,7 +267,7 @@ func (m *Manager) checkDifferentApprover(ctx context.Context, executionID string actorEmail = strings.TrimSpace(actorEmail) if actorEmail == "" { logging.Warnf("purchase[%s]: 4-eyes mode on; no actor identity available, denying (fail-closed)", executionID) - return fmt.Errorf("4-eyes approval mode is enabled but no approver identity could be determined; sign in or supply a verified actor before approving") + return fmt.Errorf("%w: 4-eyes approval mode is enabled but no approver identity could be determined; sign in or supply a verified actor before approving", ErrFourEyesDenied) } creatorEmail, err := m.config.GetUserEmailByID(ctx, *execution.CreatedByUserID) @@ -270,13 +278,13 @@ func (m *Manager) checkDifferentApprover(ctx context.Context, executionID string if creatorEmail == "" { logging.Warnf("purchase[%s]: 4-eyes mode on; creator account %s not found, denying (fail-closed)", executionID, *execution.CreatedByUserID) - return fmt.Errorf("4-eyes approval mode is enabled but the creator's account could not be resolved; an admin must investigate before approving") + return fmt.Errorf("%w: 4-eyes approval mode is enabled but the creator's account could not be resolved; an admin must investigate before approving", ErrFourEyesDenied) } if strings.EqualFold(creatorEmail, actorEmail) { logging.Warnf("purchase[%s]: 4-eyes mode on; creator %s attempted self-approval via actor %q, denied", executionID, *execution.CreatedByUserID, maskActor(actorEmail)) - return fmt.Errorf("approval declined: 4-eyes mode requires a different approver than the requester") + return fmt.Errorf("%w: 4-eyes mode requires a different approver than the requester", ErrFourEyesDenied) } return nil } @@ -297,7 +305,8 @@ func (m *Manager) checkDifferentApprover(ctx context.Context, executionID string // approval drives its own executeAndFinalize, which already fans out // per-account in parallel via executeMultiAccount. func (m *Manager) ApproveAndExecute(ctx context.Context, executionID, actor string, transitionedBy *string) (string, error) { - return m.transitionApproveAndExecute(ctx, executionID, actor, transitionedBy, []string{"pending", "notified"}) + _, revocationToken, err := m.transitionApproveAndExecute(ctx, executionID, actor, transitionedBy, []string{"pending", "notified"}, "") + return revocationToken, err } // RunPlannedPurchaseNow lets an operator force a scheduled purchase (pending @@ -316,8 +325,11 @@ func (m *Manager) ApproveAndExecute(ctx context.Context, executionID, actor stri // click, or the scheduler racing the same pending row, loses the atomic // UPDATE ... WHERE status IN (...) and gets a clean "cannot transition" // error rather than executing the purchase a second time. -func (m *Manager) RunPlannedPurchaseNow(ctx context.Context, executionID, actor string, transitionedBy *string) (string, error) { - return m.transitionApproveAndExecute(ctx, executionID, actor, transitionedBy, []string{"pending", "paused"}) +// +// The returned row carries the run-now audit stamp and its final status, also +// on an execution error; it is nil when the run never started. +func (m *Manager) RunPlannedPurchaseNow(ctx context.Context, executionID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { + return m.transitionApproveAndExecute(ctx, executionID, actor, transitionedBy, []string{"pending", "paused"}, runNowSkipReason) } // transitionApproveAndExecute is the shared body behind ApproveAndExecute @@ -332,7 +344,10 @@ func (m *Manager) RunPlannedPurchaseNow(ctx context.Context, executionID, actor // executed, revoke here" link, and since only the token's hash is stored // (issue #103) a DB re-read can never yield a raw, emailable value, so the // rotation lives here, in the one funnel all approve paths share. -func (m *Manager) transitionApproveAndExecute(ctx context.Context, executionID, actor string, transitionedBy *string, fromStatuses []string) (string, error) { +// +// A non-empty skipReason stamps the executed_* audit fields once the CAS wins. +// The returned row is nil when the CAS was never won. +func (m *Manager) transitionApproveAndExecute(ctx context.Context, executionID, actor string, transitionedBy *string, fromStatuses []string, skipReason string) (*config.PurchaseExecution, string, error) { t0 := time.Now() logging.Infof("purchase[%s]: transitionApproveAndExecute starting (actor=%q, from=%v)", executionID, maskActor(actor), fromStatuses) @@ -344,7 +359,7 @@ func (m *Manager) transitionApproveAndExecute(ctx context.Context, executionID, // enforceFourEyesPolicy's doc comment for the full rationale. if err := m.enforceFourEyesPolicy(ctx, executionID, actor, transitionedBy); err != nil { logging.Warnf("purchase[%s]: transitionApproveAndExecute denied by 4-eyes policy: %v", executionID, err) - return "", err + return nil, "", err } // transitionedBy carries the session user's UUID for human-initiated @@ -355,18 +370,27 @@ func (m *Manager) transitionApproveAndExecute(ctx context.Context, executionID, if err != nil { logging.Errorf("purchase[%s]: transitionApproveAndExecute status transition failed after %s: %v", executionID, time.Since(t0), err) - return "", fmt.Errorf("approve: %w", err) + return nil, "", fmt.Errorf("approve: %w", err) } logging.Infof("purchase[%s]: status transitioned to approved in %s", executionID, time.Since(t0)) + if skipReason != "" { + now := time.Now() + reason := skipReason + updated.ExecutedAt = &now + updated.ExecutedByUserID = transitionedBy + updated.PreApprovalSkipReason = &reason + } if actor != "" { a := actor updated.ApprovedBy = &a + } + if actor != "" || skipReason != "" { if saveErr := m.config.SavePurchaseExecution(ctx, updated); saveErr != nil { // Attribution is best-effort once the atomic flip has landed -- // dropping ApprovedBy must not stop the purchase from firing. // Log loudly so the audit gap is visible. - logging.Errorf("AUDIT GAP: failed to stamp approved_by on %s: %v", executionID, saveErr) + logging.Errorf("AUDIT GAP: failed to stamp approval audit fields on %s: %v", executionID, saveErr) } } @@ -374,7 +398,7 @@ func (m *Manager) transitionApproveAndExecute(ctx context.Context, executionID, execErr := m.executeAndFinalize(ctx, updated) if execErr != nil { logging.Errorf("purchase[%s]: transitionApproveAndExecute failed after %s: %v", executionID, time.Since(t0), execErr) - return "", execErr + return updated, "", execErr } logging.Infof("purchase[%s]: transitionApproveAndExecute completed in %s", executionID, time.Since(t0)) @@ -387,7 +411,7 @@ func (m *Manager) transitionApproveAndExecute(ctx context.Context, executionID, if mintErr != nil { logging.Warnf("purchase[%s]: transitionApproveAndExecute: revocation token mint failed (best-effort): %v", executionID, mintErr) } - return revocationToken, nil + return updated, revocationToken, nil } // CancelExecution cancels a pending execution. actor carries the email of diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index b888a9b1..72419ab0 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -330,7 +330,7 @@ func TestManager_RunPlannedPurchaseNow_ExecutesFromPaused(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-run-now", []string{"pending", "paused"}, "approved", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-run-now") - revocationToken, err := manager.RunPlannedPurchaseNow(ctx, "exec-run-now", "operator@example.com", nil) + final, revocationToken, err := manager.RunPlannedPurchaseNow(ctx, "exec-run-now", "operator@example.com", nil) require.NoError(t, err) // Run-now shares the execute funnel's revocation-token rotation (#103). require.NotEmpty(t, revocationToken) @@ -338,6 +338,8 @@ func TestManager_RunPlannedPurchaseNow_ExecutesFromPaused(t *testing.T) { assert.NotEqual(t, revocationToken, updated.ApprovalToken) require.NotNil(t, updated.ApprovedBy) assert.Equal(t, "operator@example.com", *updated.ApprovedBy) + assert.Same(t, updated, final) + assert.Equal(t, "completed", final.Status) store.AssertExpectations(t) sender.AssertExpectations(t) } @@ -355,8 +357,9 @@ func TestManager_RunPlannedPurchaseNow_LostCASReturnsError(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-race", []string{"pending", "paused"}, "approved", (*string)(nil)). Return(nil, config.ErrExecutionNotInExpectedStatus) - _, err := manager.RunPlannedPurchaseNow(ctx, "exec-race", "operator@example.com", nil) + final, _, err := manager.RunPlannedPurchaseNow(ctx, "exec-race", "operator@example.com", nil) require.Error(t, err) + assert.Nil(t, final) assert.ErrorIs(t, err, config.ErrExecutionNotInExpectedStatus) store.AssertExpectations(t) } diff --git a/internal/server/interfaces.go b/internal/server/interfaces.go index 4a56ce36..1df5c8a4 100644 --- a/internal/server/interfaces.go +++ b/internal/server/interfaces.go @@ -35,7 +35,7 @@ type PurchaseManagerInterface interface { // execute immediately (the "Run now" button), sharing ApproveAndExecute's // 4-eyes-gated, CAS-guarded funnel instead of a bare status flip that // nothing else consumes (issue #218). - RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string) (string, error) + RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) CancelExecution(ctx context.Context, execID, token, actor string) error // ReapStuckExecutions sweeps purchase_executions stuck in // approved/running longer than reapAfter and flips them to "failed" diff --git a/internal/testutil/mocks.go b/internal/testutil/mocks.go index 4c971fa4..df97421c 100644 --- a/internal/testutil/mocks.go +++ b/internal/testutil/mocks.go @@ -44,7 +44,7 @@ type MockPurchaseManager struct { ProcessMessageFunc func(ctx context.Context, body string) error ApproveExecutionFunc func(ctx context.Context, execID, token, actor string) (string, error) ApproveAndExecuteFunc func(ctx context.Context, execID, actor string, transitionedBy *string) (string, error) - RunPlannedPurchaseNowFunc func(ctx context.Context, execID, actor string, transitionedBy *string) (string, error) + RunPlannedPurchaseNowFunc func(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) CancelExecutionFunc func(ctx context.Context, execID, token, actor string) error ReapStuckExecutionsFunc func(ctx context.Context, reapAfter time.Duration) (*purchase.ReapResult, error) FireScheduledDelayedPurchasesFunc func(ctx context.Context) (*purchase.FireResult, error) @@ -86,11 +86,11 @@ func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, act return "", nil } -func (m *MockPurchaseManager) RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string) (string, error) { +func (m *MockPurchaseManager) RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { if m.RunPlannedPurchaseNowFunc != nil { return m.RunPlannedPurchaseNowFunc(ctx, execID, actor, transitionedBy) } - return "", nil + return nil, "", nil } func (m *MockPurchaseManager) CancelExecution(ctx context.Context, execID, token, actor string) error { From 8cb5c7847bea810522a046d4aa184e6c0a98ae1d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 5 Oct 2026 09:34:36 +0200 Subject: [PATCH 2/2] fix(api): map a run-now failure after the claim to 502, not 409 runNowError checked ErrNotFound / ErrExecutionNotInExpectedStatus before final == nil, so a plan deleted mid-run (or an audit-loss wrapping ErrNotFound) after the claim was reported as "cannot be started" (409) although the row was already failed or approved. Apply the 409 only when no claim was won; otherwise fall through to 502/500 with the real status. Also reword the executed_by_user_id / pre_approval_skip_reason docs: the four-eyes check runs on run-now rows and ApprovedBy is stamped. Refs #386 --- internal/api/handler_purchases.go | 3 ++- internal/api/handler_purchases_test.go | 2 ++ internal/config/types.go | 9 ++++----- internal/purchase/approvals.go | 2 +- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 57621a99..31e2b535 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -364,11 +364,12 @@ func (h *Handler) runPlannedPurchase(ctx context.Context, req *events.LambdaFunc // runNowError maps a RunPlannedPurchaseNow failure to its HTTP error. A non-nil // final means the purchase ran and money may have moved, so it is never a 409. +// ErrNotFound and ErrExecutionNotInExpectedStatus are 409 only before a claim. func runNowError(executionID string, final *config.PurchaseExecution, err error) error { switch { case errors.Is(err, purchase.ErrFourEyesDenied): return NewClientError(403, fmt.Sprintf("execution %s cannot be started: %v", executionID, err)) - case errors.Is(err, config.ErrExecutionNotInExpectedStatus), errors.Is(err, config.ErrNotFound): + case final == nil && (errors.Is(err, config.ErrExecutionNotInExpectedStatus) || errors.Is(err, config.ErrNotFound)): return NewClientError(409, fmt.Sprintf("execution %s cannot be started: %v", executionID, err)) case final == nil: return fmt.Errorf("execution %s could not be started: %w", executionID, err) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index f3df3a8e..5c2d3a42 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -2213,6 +2213,8 @@ func TestRunNowError(t *testing.T) { {"lost claim", nil, fmt.Errorf("approve: %w", config.ErrExecutionNotInExpectedStatus), 409}, {"row gone", nil, fmt.Errorf("approve: %w", config.ErrNotFound), 409}, {"store error before claim", nil, errors.New("connection reset"), 0}, + {"plan deleted after claim", ran, fmt.Errorf("failed to get plan: %w", config.ErrNotFound), 502}, + {"audit loss wrapping not found", ran, fmt.Errorf("%w: %w", config.ErrAuditLoss, config.ErrNotFound), 500}, {"final status not saved", ran, fmt.Errorf("%w: boom", config.ErrAuditLoss), 500}, {"execution failed", ran, errors.New("provider rejected"), 502}, } diff --git a/internal/config/types.go b/internal/config/types.go index ecb53209..37e5565b 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -356,17 +356,16 @@ type PurchaseExecution struct { ApprovalTokenExpiresAt *time.Time `json:"approval_token_expires_at,omitempty" dynamodbav:"approval_token_expires_at,omitempty"` // ExecutedByUserID is the UUID of the session user who triggered a // direct-execute (issue #289, execute-any/execute-own) or a run-now. NULL on rows - // that went through the normal approval flow. Non-null signals the - // approval step was intentionally skipped by an authorized operator. + // that went through the normal approval flow. Together with + // pre_approval_skip_reason it marks a skipped approval email/wait (see ApprovedBy). // Migration 000058 adds the column. ExecutedByUserID *string `json:"executed_by_user_id,omitempty" dynamodbav:"executed_by_user_id,omitempty"` // ExecutedAt is the UTC timestamp when the direct-execute or run-now path fired. // NULL for rows on the normal approval flow. Migration 000058. ExecutedAt *time.Time `json:"executed_at,omitempty" dynamodbav:"executed_at,omitempty"` // PreApprovalSkipReason is a human-readable token describing why the - // approval step was skipped. For direct-execute rows it is the literal - // string "direct-execute permission", for run-now rows "run-now". NULL on - // every normal-flow row. + // approval email/scheduled wait was skipped: "direct-execute permission" or + // "run-now". NULL on every normal-flow row. // Migration 000058. PreApprovalSkipReason *string `json:"pre_approval_skip_reason,omitempty" dynamodbav:"pre_approval_skip_reason,omitempty"` // IdempotencyKey is the stable lineage anchor the per-rec provider diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index ab49bc10..dbd04a31 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -388,7 +388,7 @@ func (m *Manager) transitionApproveAndExecute(ctx context.Context, executionID, if actor != "" || skipReason != "" { if saveErr := m.config.SavePurchaseExecution(ctx, updated); saveErr != nil { // Attribution is best-effort once the atomic flip has landed -- - // dropping ApprovedBy must not stop the purchase from firing. + // dropping these audit fields must not stop the purchase from firing. // Log loudly so the audit gap is visible. logging.Errorf("AUDIT GAP: failed to stamp approval audit fields on %s: %v", executionID, saveErr) }