From 58c803891373bfe820b305b9f741fff617818100 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 15:56:53 +0100 Subject: [PATCH 01/14] test(purchase): re-home token approve and TTL tests off the SQS message path ApproveExecution now carries the plan-id propagation test, and the shared token check gets its TTL and precedence table (wrong token on an expired row reports invalid, not expired). The SQS-entry duplicates are removed because each has a named twin in approvals_test.go that asserts the same outcome: - ApproveHappyPath: TestManager_ApproveExecution_Success (:83) - FourEyesOn_SelfApproveDenied: ..._FourEyesOn_SQSActorEqualsCreator_Denied (:713) - FourEyesOn_DifferentApproverSucceeds: ..._FourEyesOn_DifferentActor_Allowed (:757) - CancelHappyPath: TestManager_CancelExecution (:851) - ApproveRejectsTokenMismatch: ..._InvalidToken (:178) - ExpiredTokenRejectedBeforeApproverLookup: ..._ExpiredToken (:1079) and TestManager_CancelExecution_ExpiredToken (:1284) Refs #399 Co-Authored-By: Claude Opus 5.5 --- internal/purchase/approvals_test.go | 38 +++ internal/purchase/coverage_extra_test.go | 231 +------------------ internal/purchase/messages_token_ttl_test.go | 120 ---------- 3 files changed, 45 insertions(+), 344 deletions(-) delete mode 100644 internal/purchase/messages_token_ttl_test.go diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index 41110f069..c7ab7aa0c 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -1498,3 +1498,41 @@ func TestManager_ApproveExecution_FourEyesOn_ASCIICaseOnly(t *testing.T) { }) } } + +// TestValidateApprovalToken_TTLAndPrecedence pins the TTL guard on the shared +// token check (issue #268) and that a wrong token on an expired row reports +// invalid, not expired, so expiry never leaks token validity. +func TestValidateApprovalToken_TTLAndPrecedence(t *testing.T) { + past := time.Now().Add(-time.Hour) + future := time.Now().Add(time.Hour) + + tests := []struct { + name string + expiresAt *time.Time + token string + wantErr error + }{ + {name: "expired token is rejected", expiresAt: &past, token: "correct-token", wantErr: ErrApprovalTokenExpired}, + {name: "unexpired token is accepted", expiresAt: &future, token: "correct-token"}, + {name: "legacy row without expiry is accepted", expiresAt: nil, token: "correct-token"}, + {name: "wrong token is rejected", expiresAt: &future, token: "wrong-token", wantErr: ErrInvalidApprovalToken}, + {name: "wrong token on expired row reports invalid, not expired", expiresAt: &past, token: "wrong-token", wantErr: ErrInvalidApprovalToken}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + exec := &config.PurchaseExecution{ + ExecutionID: "exec-ttl", + Status: "pending", + ApprovalToken: config.HashApprovalToken("correct-token"), + ApprovalTokenExpiresAt: tt.expiresAt, + } + err := ValidateApprovalToken(exec, tt.token) + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + return + } + require.NoError(t, err) + }) + } +} diff --git a/internal/purchase/coverage_extra_test.go b/internal/purchase/coverage_extra_test.go index f742c7fe1..bdf6aec97 100644 --- a/internal/purchase/coverage_extra_test.go +++ b/internal/purchase/coverage_extra_test.go @@ -3,7 +3,6 @@ package purchase import ( "context" "errors" - "fmt" "testing" "time" @@ -368,11 +367,11 @@ func TestHandleExecutePurchase_SaveError(t *testing.T) { assert.ErrorIs(t, err, config.ErrAuditLoss) } -// Tests for ProcessMessage with approve and cancel happy paths. -// Post-hardening: SQS approve/cancel messages MUST carry actor_email, -// and the actor MUST match a per-account contact_email on one of the -// execution's recommendations. The mocks below set up that gating. -func TestProcessMessage_ApproveHappyPath(t *testing.T) { +// TestManager_ApproveExecution_PropagatesPlanID pins that a non-empty PlanID +// reaches GetPurchasePlan through the approve->execute chain. Earlier versions +// used "" and would have silently passed even if the plan id were dropped or +// substituted en route. +func TestManager_ApproveExecution_PropagatesPlanID(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) mockEmail := new(MockEmailSender) @@ -402,13 +401,10 @@ func TestProcessMessage_ApproveHappyPath(t *testing.T) { StepNumber: 1, Recommendations: exec.Recommendations, } - account := &config.CloudAccount{ID: accountID, ContactEmail: "owner@example.com"} - // verifyAsyncApprovalActor + ApproveExecution both load the execution. // rotateApprovalToken adds no further fetch: it mutates `approved` (the // row TransitionExecutionStatus returns) in place. - mockStore.On("GetExecutionByID", ctx, "exec-appv").Return(exec, nil).Times(2) - mockStore.On("GetCloudAccount", ctx, accountID).Return(account, nil) + mockStore.On("GetExecutionByID", ctx, "exec-appv").Return(exec, nil).Times(1) // Atomic approve transition (issue #372 fix). mockStore.On("TransitionExecutionStatusTokenPinned", ctx, "exec-appv", []string{"pending", "notified"}, "running", (*string)(nil), "correct-token").Return(approved, nil) // Synchronous execute chain: GetPurchasePlan is called with the @@ -428,83 +424,13 @@ func TestProcessMessage_ApproveHappyPath(t *testing.T) { dashboardURL: "https://dashboard.example.com", } - err := manager.ProcessMessage(ctx, `{"type":"approve","execution_id":"exec-appv","token":"correct-token","actor_email":"owner@example.com"}`) + _, _, err := manager.ApproveExecution(ctx, "exec-appv", "correct-token", "owner@example.com", nil) require.NoError(t, err) mockStore.AssertExpectations(t) mockEmail.AssertNotCalled(t, "SendPurchaseConfirmation", mock.Anything, mock.Anything) mockEmail.AssertExpectations(t) } -// TestProcessMessage_ApproveFourEyesOn_SelfApproveDenied is the true -// end-to-end regression test for the MEDIUM finding on PR #1500's -// adversarial review: the SQS approve worker (handleApproveMessage -> -// verifyAsyncApprovalActor -> ApproveExecution) enforced token + actor_email -// + per-account contact_email matching, but never consulted 4-eyes mode. -// A replayed or forwarded {token, actor_email} pair belonging to the same -// person who created the execution would approve it even with dual control -// enabled. Pre-fix this test proceeds straight through to -// TransitionExecutionStatus (proven by stashing the fix and re-running); -// post-fix ProcessMessage returns the 4-eyes denial before any state change. -// -// The case-differing row (issue #307) runs both email folds on the SQS path -// end to end: the actor must match the mixed-case per-account contact_email -// case-insensitively to get past verifyAsyncApprovalActor, and must then be -// recognized as the creator despite the case difference so 4-eyes denies. -func TestProcessMessage_ApproveFourEyesOn_SelfApproveDenied(t *testing.T) { - cases := []struct { - name string - storedEmail string // creator's user email and the account contact_email - messageActor string - }{ - {name: "byte-identical emails", storedEmail: "owner@example.com", messageActor: "owner@example.com"}, - {name: "stored mixed case, actor lowercase", storedEmail: "Owner@Example.COM", messageActor: "owner@example.com"}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - mockStore.Test(t) // fail this row on an unexpected call, don't panic - mockEmail := new(MockEmailSender) - - accountID := "acct-1" - creatorID := "user-creator" - exec := &config.PurchaseExecution{ - ExecutionID: "exec-appv-self", - Status: "pending", - ApprovalToken: config.HashApprovalToken("correct-token"), - CreatedByUserID: &creatorID, - Recommendations: []config.RecommendationRecord{ - {CloudAccountID: &accountID}, - }, - } - account := &config.CloudAccount{ID: accountID, ContactEmail: tc.storedEmail} - - // verifyAsyncApprovalActor + the manager's own fetch inside - // ApproveExecution both load the execution; enforceFourEyesPolicy - // loads it again. - mockStore.On("GetExecutionByID", ctx, "exec-appv-self").Return(exec, nil) - mockStore.On("GetCloudAccount", ctx, accountID).Return(account, nil) - mockStore.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) - mockStore.On("GetUserEmailByID", ctx, creatorID).Return(tc.storedEmail, nil) - - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - - msg := fmt.Sprintf(`{"type":"approve","execution_id":"exec-appv-self","token":"correct-token","actor_email":%q}`, tc.messageActor) - err := manager.ProcessMessage(ctx, msg) - require.ErrorIs(t, err, ErrFourEyesDenied) - assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") - mockStore.AssertNotCalled(t, "TransitionExecutionStatusTokenPinned", - mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) - mockStore.AssertExpectations(t) - mockEmail.AssertExpectations(t) - }) - } -} - // TestProcessMessage_ApproverMatchIgnoresEmailCase pins the case-insensitive // comparison in matchActorAgainstApprovers (issue #307): an SQS approve or // cancel message whose actor_email differs from the account's stored @@ -550,108 +476,6 @@ func TestProcessMessage_ApproverMatchIgnoresEmailCase(t *testing.T) { } } -// TestProcessMessage_ApproveFourEyesOn_DifferentApproverSucceeds is the -// positive control for the same SQS call shape: an actor_email verified -// against the per-account contact_email list, but belonging to a different -// person than the creator, must still succeed end to end. -func TestProcessMessage_ApproveFourEyesOn_DifferentApproverSucceeds(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - - accountID := "acct-1" - planID := "plan-appv-diff" - creatorID := "user-creator" - exec := &config.PurchaseExecution{ - ExecutionID: "exec-appv-diff", - PlanID: planID, - Status: "pending", - ApprovalToken: config.HashApprovalToken("correct-token"), - CreatedByUserID: &creatorID, - Recommendations: []config.RecommendationRecord{ - {CloudAccountID: &accountID}, - }, - } - approved := &config.PurchaseExecution{ - ExecutionID: "exec-appv-diff", - PlanID: planID, - Status: "approved", - StepNumber: 1, - Recommendations: exec.Recommendations, - } - account := &config.CloudAccount{ID: accountID, ContactEmail: "approver@example.com"} - - // verifyAsyncApprovalActor + ApproveExecution's own load + the - // enforceFourEyesPolicy load (issue #1005 gate): 3 loads total. - // rotateApprovalToken adds no further fetch: it mutates `approved` (the - // row TransitionExecutionStatus returns) in place. - mockStore.On("GetExecutionByID", ctx, "exec-appv-diff").Return(exec, nil).Times(3) - mockStore.On("GetCloudAccount", ctx, accountID).Return(account, nil) - mockStore.On("GetGlobalConfig", mock.Anything).Return(fourEyesCfgOnForManager(), nil) - mockStore.On("GetUserEmailByID", ctx, creatorID).Return("owner@example.com", nil) - mockStore.On("TransitionExecutionStatusTokenPinned", ctx, "exec-appv-diff", []string{"pending", "notified"}, "running", (*string)(nil), "correct-token").Return(approved, nil) - plan := &config.PurchasePlan{ID: planID, Name: "test-plan"} - mockStore.On("GetPurchasePlan", mock.Anything, planID).Return(plan, 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) - - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - - 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) -} - -func TestProcessMessage_CancelHappyPath(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - - accountID := "acct-1" - exec := &config.PurchaseExecution{ - ExecutionID: "exec-cancel", - Status: "pending", - ApprovalToken: config.HashApprovalToken("correct-token"), - Recommendations: []config.RecommendationRecord{ - {CloudAccountID: &accountID}, - }, - } - account := &config.CloudAccount{ID: accountID, ContactEmail: "owner@example.com"} - - // verifyAsyncApprovalActor + CancelExecution + rotateApprovalToken all - // load the execution; mock returns it three times. - mockStore.On("GetExecutionByID", ctx, "exec-cancel").Return(exec, nil).Times(3) - mockStore.On("GetCloudAccount", ctx, accountID).Return(account, nil) - // CancelExecutionAtomicTokenPinned is called inside WithTx (nil tx - // sentinel in tests); actor_email is non-empty so cancelledBy is non-nil. - actor := "owner@example.com" - mockStore.On("CancelExecutionAtomicTokenPinned", ctx, mock.Anything, "exec-cancel", &actor, "correct-token"). - Return(true, "canceled", nil) - // Suppression cleanup must follow a successful atomic cancel. - mockStore.On("DeleteSuppressionsByExecutionTx", ctx, mock.Anything, "exec-cancel"). - Return(nil) - // Token rotation after successful cancel. - mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) - - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - - err := manager.ProcessMessage(ctx, `{"type":"cancel","execution_id":"exec-cancel","token":"correct-token","actor_email":"owner@example.com"}`) - require.NoError(t, err) - mockStore.AssertExpectations(t) -} - // TestProcessMessage_ApproveRejectsMissingActor verifies the SQS // approve handler refuses to process a message without an actor_email. // This is the regression test for the closed bypass: legacy / replayed @@ -733,47 +557,6 @@ func TestProcessMessage_ApproveRejectsNonMatchingActor(t *testing.T) { mockStore.AssertNotCalled(t, "SavePurchaseExecution") } -// TestProcessMessage_ApproveRejectsTokenMismatch: token is wrong, -// actor_email is set. Existing behavior (token check fails) — verified -// here so a future refactor of verifyAsyncApprovalActor doesn't -// regress the token comparison ordering. -func TestProcessMessage_ApproveRejectsTokenMismatch(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - assertMocksOnCleanup(t, mockStore) - mockEmail := new(MockEmailSender) - - // The recommendation carries a CloudAccountID so that approver resolution, - // if it were ever hoisted above the token check, would have an account to - // look up. Without it the AssertNotCalled below is inert: the ordering bug - // it guards could not produce a GetCloudAccount call against this fixture. - accountID := "acct-1" - exec := &config.PurchaseExecution{ - ExecutionID: "exec-bad-token", - Status: "pending", - ApprovalToken: config.HashApprovalToken("correct-token"), - Recommendations: []config.RecommendationRecord{ - {CloudAccountID: &accountID}, - }, - } - - mockStore.On("GetExecutionByID", ctx, "exec-bad-token").Return(exec, nil) - - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - - err := manager.ProcessMessage(ctx, `{"type":"approve","execution_id":"exec-bad-token","token":"wrong-token","actor_email":"owner@example.com"}`) - require.Error(t, err) - assert.Contains(t, err.Error(), "invalid approval token") - // No GetCloudAccount (token failure is checked before approver - // resolution) and no SavePurchaseExecution. - mockStore.AssertNotCalled(t, "GetCloudAccount") - mockStore.AssertNotCalled(t, "SavePurchaseExecution") -} - // TestProcessMessage_ApproveRejectsNoApprovers: token + actor are valid // in shape, but no recommendation references an account with a // configured contact_email. Same policy as the HTTP path: the global diff --git a/internal/purchase/messages_token_ttl_test.go b/internal/purchase/messages_token_ttl_test.go deleted file mode 100644 index 3b0750619..000000000 --- a/internal/purchase/messages_token_ttl_test.go +++ /dev/null @@ -1,120 +0,0 @@ -package purchase - -import ( - "context" - "testing" - "time" - - "github.com/LeanerCloud/cloud-commitments-platform/internal/config" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" -) - -// Issue #268: loadAsyncExecutionForApproval was a third copy of the -// approval-token check (alongside ValidateApprovalToken and -// loadCancelableExecution) that compared the token but never enforced -// ApprovalTokenExpiresAt. An expired token on an SQS approve/cancel message -// was only rejected because ApproveExecution / CancelExecution re-validated -// it one frame later. These tests pin the TTL guard on the helper itself and -// on the SQS entry points, before approver resolution runs. - -func asyncTTLExecution(id string, expiresAt *time.Time) *config.PurchaseExecution { - accountID := "acct-1" - return &config.PurchaseExecution{ - ExecutionID: id, - Status: "pending", - ApprovalToken: config.HashApprovalToken("correct-token"), - ApprovalTokenExpiresAt: expiresAt, - Recommendations: []config.RecommendationRecord{ - {CloudAccountID: &accountID}, - }, - } -} - -func TestLoadAsyncExecutionForApproval_TokenValidation(t *testing.T) { - past := time.Now().Add(-time.Hour) - future := time.Now().Add(time.Hour) - - tests := []struct { - name string - expiresAt *time.Time - token string - wantErr error - }{ - {name: "expired token is rejected", expiresAt: &past, token: "correct-token", wantErr: ErrApprovalTokenExpired}, - {name: "unexpired token is accepted", expiresAt: &future, token: "correct-token"}, - {name: "legacy row without expiry is accepted", expiresAt: nil, token: "correct-token"}, - {name: "wrong token is rejected", expiresAt: &future, token: "wrong-token", wantErr: ErrInvalidApprovalToken}, - {name: "wrong token on expired row reports invalid, not expired", expiresAt: &past, token: "wrong-token", wantErr: ErrInvalidApprovalToken}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - exec := asyncTTLExecution("exec-ttl", tt.expiresAt) - mockStore.On("GetExecutionByID", ctx, "exec-ttl").Return(exec, nil).Once() - - manager := &Manager{config: mockStore} - got, err := manager.loadAsyncExecutionForApproval(ctx, &AsyncMessage{ - ExecutionID: "exec-ttl", - Token: tt.token, - }) - - if tt.wantErr != nil { - require.ErrorIs(t, err, tt.wantErr) - assert.Nil(t, got) - } else { - require.NoError(t, err) - assert.Same(t, exec, got) - } - mockStore.AssertExpectations(t) - }) - } -} - -// TestProcessMessage_ExpiredTokenRejectedBeforeApproverLookup drives the -// real SQS entry point for both approve and cancel with an expired token -// from an actor who IS on the approver list. The rejection must come from -// verifyAsyncApprovalActor's own token check: the execution is loaded once -// and approver resolution (GetCloudAccount) never runs. Before the fix the -// approver lookup succeeded and the message was only rejected by the second -// load inside ApproveExecution / CancelExecution. -func TestProcessMessage_ExpiredTokenRejectedBeforeApproverLookup(t *testing.T) { - for _, msgType := range []string{"approve", "cancel"} { - t.Run(msgType, func(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - - past := time.Now().Add(-24 * time.Hour) - exec := asyncTTLExecution("exec-expired", &past) - // GetExecutionByID and GetCloudAccount are registered without a - // call limit so a pre-fix run reaches the approver match and the - // second load inside ApproveExecution / CancelExecution instead - // of panicking on an unexpected call; the call-count and - // AssertNotCalled checks below are the actual assertions. - mockStore.On("GetExecutionByID", ctx, "exec-expired").Return(exec, nil) - account := &config.CloudAccount{ID: "acct-1", ContactEmail: "owner@example.com"} - mockStore.On("GetCloudAccount", ctx, "acct-1").Return(account, nil).Maybe() - - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - - err := manager.ProcessMessage(ctx, `{"type":"`+msgType+`","execution_id":"exec-expired","token":"correct-token","actor_email":"owner@example.com"}`) - require.ErrorIs(t, err, ErrApprovalTokenExpired) - mockStore.AssertNumberOfCalls(t, "GetExecutionByID", 1) - mockStore.AssertNotCalled(t, "GetCloudAccount", mock.Anything, mock.Anything) - mockStore.AssertNotCalled(t, "TransitionExecutionStatusTokenPinned", - mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) - mockStore.AssertNotCalled(t, "CancelExecutionAtomicTokenPinned", - mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) - mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) - mockStore.AssertExpectations(t) - }) - } -} From b0d54747e7a26f1b674e06d936f4546d4a121681 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 15:57:09 +0100 Subject: [PATCH 02/14] test(api): pin case-insensitive approver match on the HTTP gate The case and whitespace insensitive comparison was tested only through the SQS approver check. It is now asserted on authorizeApprovalAction, the production gate, for both sides of the comparison. The SQS-only tests are removed: the missing-actor cases exist only because the SQS message carried the actor (the HTTP twin is TestHandler_approvePurchase_RejectsMissingContactEmail), the mismatch and non-ASCII cases are covered by TestHandler_authorizeApprovalAction_NonASCIILettersNotFolded and the no-approver case by the "no per-account contact email" assertions. Refs #399 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_purchases_test.go | 30 ++++ internal/purchase/coverage_extra_test.go | 194 ----------------------- 2 files changed, 30 insertions(+), 194 deletions(-) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 6f990c901..099816aca 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -7068,3 +7068,33 @@ func TestHandler_authorizeApprovalAction_NonASCIILettersNotFolded(t *testing.T) }) } } + +// The approver comparison ignores letter case and surrounding whitespace on +// either side (issue #307), so a session email that differs from the stored +// contact_email only by case is the authorized approver. +func TestHandler_authorizeApprovalAction_ApproverMatchIgnoresEmailCase(t *testing.T) { + cases := []struct{ name, contact, session string }{ + {"mixed_case_contact_lower_session", " Owner@Example.COM ", "owner@example.com"}, + {"lower_contact_mixed_case_session", "owner@example.com", "Owner@Example.COM"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + execID := "12345678-1234-1234-1234-123456789abc" + + mockConfig := new(MockConfigStore) + exec := approvalTestExec(execID, tc.contact, mockConfig) + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: tc.session}, nil) + + handler := &Handler{config: mockConfig, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer sess-tok"}} + + actor, err := handler.authorizeApprovalAction(ctx, req, exec) + require.NoError(t, err) + assert.Equal(t, tc.session, actor) + }) + } +} diff --git a/internal/purchase/coverage_extra_test.go b/internal/purchase/coverage_extra_test.go index bdf6aec97..67cb26c0b 100644 --- a/internal/purchase/coverage_extra_test.go +++ b/internal/purchase/coverage_extra_test.go @@ -431,163 +431,6 @@ func TestManager_ApproveExecution_PropagatesPlanID(t *testing.T) { mockEmail.AssertExpectations(t) } -// TestProcessMessage_ApproverMatchIgnoresEmailCase pins the case-insensitive -// comparison in matchActorAgainstApprovers (issue #307): an SQS approve or -// cancel message whose actor_email differs from the account's stored -// contact_email only in letter case is the authorized approver and must get -// past verifyAsyncApprovalActor. One row per side of the comparison, so -// dropping the strings.ToLower on either the actor or the stored address -// fails a row. The non-matching control -// (TestProcessMessage_ApproveRejectsNonMatchingActor) keeps an "accept -// everyone" regression from passing. -func TestProcessMessage_ApproverMatchIgnoresEmailCase(t *testing.T) { - cases := []struct { - name string - contactEmail string - messageActor string - }{ - {name: "contact_email mixed case, actor lowercase", contactEmail: "Owner@Example.COM", messageActor: "owner@example.com"}, - {name: "contact_email lowercase, actor mixed case", contactEmail: "owner@example.com", messageActor: "OWNER@Example.com"}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - mockStore.Test(t) // fail this row on an unexpected call, don't panic - - accountID := "acct-1" - exec := &config.PurchaseExecution{ - ExecutionID: "exec-case", - Status: "pending", - ApprovalToken: config.HashApprovalToken("correct-token"), - Recommendations: []config.RecommendationRecord{ - {CloudAccountID: &accountID}, - }, - } - mockStore.On("GetExecutionByID", ctx, "exec-case").Return(exec, nil) - mockStore.On("GetCloudAccount", ctx, accountID). - Return(&config.CloudAccount{ID: accountID, ContactEmail: tc.contactEmail}, nil) - - manager := &Manager{config: mockStore, email: new(MockEmailSender)} - msg := &AsyncMessage{ExecutionID: "exec-case", Token: "correct-token", ActorEmail: tc.messageActor} - require.NoError(t, manager.verifyAsyncApprovalActor(ctx, msg)) - mockStore.AssertExpectations(t) - }) - } -} - -// TestProcessMessage_ApproveRejectsMissingActor verifies the SQS -// approve handler refuses to process a message without an actor_email. -// This is the regression test for the closed bypass: legacy / replayed -// payloads without the field must NOT fall through to a tokenless -// approval. -func TestProcessMessage_ApproveRejectsMissingActor(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - - err := manager.ProcessMessage(ctx, `{"type":"approve","execution_id":"exec-x","token":"some-token"}`) - require.Error(t, err) - assert.Contains(t, err.Error(), "actor_email required") - // No store calls should have happened — no approval, no save. - mockStore.AssertNotCalled(t, "GetExecutionByID") - mockStore.AssertNotCalled(t, "SavePurchaseExecution") -} - -// TestProcessMessage_CancelRejectsMissingActor mirrors the approve -// guard for the cancel path. -func TestProcessMessage_CancelRejectsMissingActor(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - - err := manager.ProcessMessage(ctx, `{"type":"cancel","execution_id":"exec-x","token":"some-token"}`) - require.Error(t, err) - assert.Contains(t, err.Error(), "actor_email required") - mockStore.AssertNotCalled(t, "GetExecutionByID") - mockStore.AssertNotCalled(t, "SavePurchaseExecution") -} - -// TestProcessMessage_ApproveRejectsNonMatchingActor: token is valid, -// actor_email is present but not on the per-account contact_email -// approver list. Reject — same outcome as the HTTP path's -// authorizeApprovalAction. -func TestProcessMessage_ApproveRejectsNonMatchingActor(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - assertMocksOnCleanup(t, mockStore) - mockEmail := new(MockEmailSender) - - accountID := "acct-1" - exec := &config.PurchaseExecution{ - ExecutionID: "exec-mismatch", - Status: "pending", - ApprovalToken: config.HashApprovalToken("correct-token"), - Recommendations: []config.RecommendationRecord{ - {CloudAccountID: &accountID}, - }, - } - account := &config.CloudAccount{ID: accountID, ContactEmail: "owner@example.com"} - - mockStore.On("GetExecutionByID", ctx, "exec-mismatch").Return(exec, nil) - mockStore.On("GetCloudAccount", ctx, accountID).Return(account, nil) - - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - - err := manager.ProcessMessage(ctx, `{"type":"approve","execution_id":"exec-mismatch","token":"correct-token","actor_email":"intruder@example.com"}`) - require.Error(t, err) - assert.Contains(t, err.Error(), "not an authorized approver") - // SavePurchaseExecution must NOT have been called — no mutation. - mockStore.AssertNotCalled(t, "SavePurchaseExecution") -} - -// TestProcessMessage_ApproveRejectsNoApprovers: token + actor are valid -// in shape, but no recommendation references an account with a -// configured contact_email. Same policy as the HTTP path: the global -// notify mailbox is not an approver — reject. -func TestProcessMessage_ApproveRejectsNoApprovers(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - assertMocksOnCleanup(t, mockStore) - mockEmail := new(MockEmailSender) - - exec := &config.PurchaseExecution{ - ExecutionID: "exec-no-approvers", - Status: "pending", - ApprovalToken: config.HashApprovalToken("correct-token"), - Recommendations: []config.RecommendationRecord{}, // no account refs - } - - mockStore.On("GetExecutionByID", ctx, "exec-no-approvers").Return(exec, nil) - - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - - err := manager.ProcessMessage(ctx, `{"type":"approve","execution_id":"exec-no-approvers","token":"correct-token","actor_email":"owner@example.com"}`) - require.Error(t, err) - assert.Contains(t, err.Error(), "no per-account contact email configured") - mockStore.AssertNotCalled(t, "SavePurchaseExecution") -} - // Tests for executeSinglePurchase error paths via executePurchase func TestManager_ExecuteSinglePurchase_ProviderError(t *testing.T) { @@ -1431,40 +1274,3 @@ func TestExecuteForAccount_NonHostRoleARN_StoredErrorOmitsHostIdentity(t *testin } assert.Contains(t, saved[len(saved)-1].Error, credentials.ErrNotHostAccount.Error()) } - -// TestProcessMessage_Approve_NonASCIILettersNotFolded pins that the approver -// match treats a differently-spelled address as a different address. -func TestProcessMessage_Approve_NonASCIILettersNotFolded(t *testing.T) { - cases := []struct{ name, contact, actorJSON string }{ - {"long_s", "mus@example.com", `mu\u017f@example.com`}, - {"dotted_capital_i", "admin@example.com", `adm\u0130n@example.com`}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - - accountID := "acct-1" - exec := &config.PurchaseExecution{ - ExecutionID: "exec-cmp", - Status: "pending", - ApprovalToken: config.HashApprovalToken("correct-token"), - Recommendations: []config.RecommendationRecord{ - {CloudAccountID: &accountID}, - }, - } - account := &config.CloudAccount{ID: accountID, ContactEmail: tc.contact} - - mockStore.On("GetExecutionByID", ctx, "exec-cmp").Return(exec, nil) - mockStore.On("GetCloudAccount", ctx, accountID).Return(account, nil) - - manager := &Manager{config: mockStore, email: mockEmail, dashboardURL: "https://dashboard.example.com"} - - err := manager.ProcessMessage(ctx, `{"type":"approve","execution_id":"exec-cmp","token":"correct-token","actor_email":"`+tc.actorJSON+`"}`) - require.Error(t, err) - assert.Contains(t, err.Error(), "not an authorized approver") - mockStore.AssertNotCalled(t, "SavePurchaseExecution") - }) - } -} From 7819496d3b1d0833eb57b9a0255636c18ec7fe41 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 15:57:25 +0100 Subject: [PATCH 03/14] test(purchase): drive the execute-path tests through the cron sweep and claim funnel Multi-account ack, redelivery and failure-notification tests now run through ProcessScheduledPurchases (Executed counts the ack), and the claim-pin, save-error and success tests call claimAndExecuteWithin directly with notified rows. The SQS execute_purchase refusal table is removed (every case is a row of TestScheduler_RefusesBeforeClaim), as are the by-ID load error test, the approved-row gate bypass test and the approved-row cap-skip test, which only exercised the SQS-only rules. A funnel-level test keeps the ErrStepCommitmentCap assertion. Refs #399 Co-Authored-By: Claude Opus 5.5 --- internal/purchase/claim_pin_test.go | 156 ++++++++---------- internal/purchase/coverage_extra_test.go | 72 +++----- internal/purchase/failure_notify_test.go | 11 +- .../purchase/failure_notify_writers_test.go | 9 +- .../purchase/money_path_regression_test.go | 46 ++++-- internal/purchase/step_cap_test.go | 60 +------ 6 files changed, 139 insertions(+), 215 deletions(-) diff --git a/internal/purchase/claim_pin_test.go b/internal/purchase/claim_pin_test.go index 8a6fe18cd..dd7c3075e 100644 --- a/internal/purchase/claim_pin_test.go +++ b/internal/purchase/claim_pin_test.go @@ -19,17 +19,13 @@ import ( var errPinLost = errors.New("pin lost") -// newPinManager wires store for one due plan row on the cron and SQS paths. The -// plan the gate reads is gatePlan; every later GetPurchasePlan read (the -// post-claim re-check) returns claimPlan. +// newPinManager wires store for one due plan row on the cron path. The plan the +// gate reads is gatePlan; every later GetPurchasePlan read (the post-claim +// re-check) returns claimPlan. The claim and save calls each test adds are the +// required ones. func newPinManager(ctx context.Context, store *MockConfigStore, factory *MockProviderFactory, exec config.PurchaseExecution, gatePlan, claimPlan *config.PurchasePlan) *Manager { - // Entry reads are per path: the cron path lists the stale and pending rows, - // the SQS path loads the row by ID. A test drives one path, so the other - // path's reads stay unused (.Maybe). The claim and save calls each test adds - // are the required ones. store.On("GetStaleApprovedExecutions", ctx, mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() store.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{exec}, nil).Maybe() - store.On("GetExecutionByID", ctx, exec.ExecutionID).Return(&exec, nil).Maybe() store.On("GetPurchasePlan", ctx, exec.PlanID).Return(gatePlan, nil).Once() store.On("GetPurchasePlan", ctx, exec.PlanID).Return(claimPlan, nil) store.On("GetGlobalConfig", ctx).Return(autoPurchaseOnCfg(), nil) @@ -43,32 +39,24 @@ func autoPlan(c *float64) *config.PurchasePlan { } // S2: an attach landing between the gate (which saw an empty row) and the claim -// makes the pinned claim lose; nothing runs, on both the cron and SQS paths. +// makes the pinned claim lose; nothing runs. func TestClaimPin_AttachBetweenGateAndClaimIsRefused(t *testing.T) { ctx := context.Background() - for _, path := range []string{"cron", "sqs"} { - t.Run(path, func(t *testing.T) { - store, factory := new(MockConfigStore), new(MockProviderFactory) - assertMocksOnCleanup(t, store, factory) - manager := newPinManager(ctx, store, factory, dueRecExec("notified"), autoPlan(f64(1000)), autoPlan(f64(1000))) - store.On("TransitionExecutionStatusPinned", ctx, "exec-gate", []string{"approved", "pending", "notified"}, "running", (*string)(nil), true). - Return(nil, config.ErrExecutionNotInExpectedStatus) - - if path == "cron" { - result, err := manager.ProcessScheduledPurchases(ctx) - require.NoError(t, err) - assert.Equal(t, 0, result.Executed) - assert.Equal(t, 0, result.Failed, "a lost pin is a benign skip") - } else { - require.NoError(t, manager.handleExecutePurchase(ctx, &AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: "exec-gate"})) - } - - store.AssertCalled(t, "TransitionExecutionStatusPinned", ctx, "exec-gate", []string{"approved", "pending", "notified"}, "running", (*string)(nil), true) - store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) - store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) - factory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) - }) - } + store, factory := new(MockConfigStore), new(MockProviderFactory) + assertMocksOnCleanup(t, store, factory) + manager := newPinManager(ctx, store, factory, dueRecExec("notified"), autoPlan(f64(1000)), autoPlan(f64(1000))) + store.On("TransitionExecutionStatusPinned", ctx, "exec-gate", []string{"approved", "pending", "notified"}, "running", (*string)(nil), true). + Return(nil, config.ErrExecutionNotInExpectedStatus) + + result, err := manager.ProcessScheduledPurchases(ctx) + require.NoError(t, err) + assert.Equal(t, 0, result.Executed) + assert.Equal(t, 0, result.Failed, "a lost pin is a benign skip") + + store.AssertCalled(t, "TransitionExecutionStatusPinned", ctx, "exec-gate", []string{"approved", "pending", "notified"}, "running", (*string)(nil), true) + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + factory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) } // A row the gate saw WITH recommendations is claimed with the opposite pin. @@ -104,38 +92,53 @@ func TestClaimPin_ClaimErrorIsFailure(t *testing.T) { // call is made. func TestClaimPin_PostClaimCapRecheckFailsBeforeSubmission(t *testing.T) { ctx := context.Background() - for _, path := range []string{"cron", "sqs"} { - t.Run(path, func(t *testing.T) { - exec := dueRecExec("notified", upfrontRec(10)) - exec.Recommendations[0].ID = "rec-1" - store, factory := new(MockConfigStore), new(MockProviderFactory) - assertMocksOnCleanup(t, store, factory) - manager := newPinManager(ctx, store, factory, exec, autoPlan(f64(1000)), autoPlan(f64(5))) - claimed := exec - claimed.Status = "running" - store.On("TransitionExecutionStatusPinned", ctx, "exec-gate", mock.Anything, "running", (*string)(nil), false).Return(&claimed, nil) - var saved *config.PurchaseExecution - store.On("SaveExecutionIfStatusTx", ctx, mock.Anything, mock.Anything, []string{"running"}). - Run(func(args mock.Arguments) { saved = args.Get(2).(*config.PurchaseExecution) }).Return(nil) - store.On("DeleteSuppressionsByExecutionTx", ctx, mock.Anything, "exec-gate").Return(nil) - - if path == "cron" { - result, err := manager.ProcessScheduledPurchases(ctx) - require.NoError(t, err) - assert.Equal(t, 1, result.Failed) - assert.Equal(t, 0, result.Executed) - } else { - err := manager.handleExecutePurchase(ctx, &AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: "exec-gate"}) - require.ErrorIs(t, err, ErrStepCommitmentCap) - } - - require.NotNil(t, saved) - assert.Equal(t, "failed", saved.Status) - assert.Contains(t, saved.Error, "exceeds max_step_commitment") - store.AssertCalled(t, "DeleteSuppressionsByExecutionTx", ctx, mock.Anything, "exec-gate") - factory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) - }) - } + exec := dueRecExec("notified", upfrontRec(10)) + exec.Recommendations[0].ID = "rec-1" + store, factory := new(MockConfigStore), new(MockProviderFactory) + assertMocksOnCleanup(t, store, factory) + manager := newPinManager(ctx, store, factory, exec, autoPlan(f64(1000)), autoPlan(f64(5))) + claimed := exec + claimed.Status = "running" + store.On("TransitionExecutionStatusPinned", ctx, "exec-gate", mock.Anything, "running", (*string)(nil), false).Return(&claimed, nil) + var saved *config.PurchaseExecution + store.On("SaveExecutionIfStatusTx", ctx, mock.Anything, mock.Anything, []string{"running"}). + Run(func(args mock.Arguments) { saved = args.Get(2).(*config.PurchaseExecution) }).Return(nil) + store.On("DeleteSuppressionsByExecutionTx", ctx, mock.Anything, "exec-gate").Return(nil) + + result, err := manager.ProcessScheduledPurchases(ctx) + require.NoError(t, err) + assert.Equal(t, 1, result.Failed) + assert.Equal(t, 0, result.Executed) + + require.NotNil(t, saved) + assert.Equal(t, "failed", saved.Status) + assert.Contains(t, saved.Error, "exceeds max_step_commitment") + store.AssertCalled(t, "DeleteSuppressionsByExecutionTx", ctx, mock.Anything, "exec-gate") + factory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) +} + +// The claim funnel itself returns the cap refusal, so a caller that gates a row +// by other means still gets ErrStepCommitmentCap and no provider call. +func TestClaimPin_ClaimFunnelReturnsCapRefusal(t *testing.T) { + ctx := context.Background() + exec := dueRecExec("notified", upfrontRec(10)) + exec.Recommendations[0].ID = "rec-1" + store, factory := new(MockConfigStore), new(MockProviderFactory) + sender := allowFailedNotification(new(MockEmailSender)) + assertMocksOnCleanup(t, store, factory, sender) + store.On("GetPurchasePlan", ctx, exec.PlanID).Return(autoPlan(f64(5)), nil) + store.On("GetGlobalConfig", mock.Anything).Return(autoPurchaseOnCfg(), nil).Maybe() + claimed := exec + claimed.Status = "running" + store.On("TransitionExecutionStatusPinned", ctx, "exec-gate", mock.Anything, "running", (*string)(nil), false).Return(&claimed, nil) + store.On("SaveExecutionIfStatusTx", ctx, mock.Anything, mock.Anything, []string{"running"}).Return(nil) + store.On("DeleteSuppressionsByExecutionTx", ctx, mock.Anything, "exec-gate").Return(nil) + manager := &Manager{config: store, email: sender, providerFactory: factory} + + didClaim, err := manager.claimAndExecuteWithin(ctx, &exec, 0) + require.True(t, didClaim) + require.ErrorIs(t, err, ErrStepCommitmentCap) + factory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) } // A post-claim plan read failure also fails the claimed row closed rather than @@ -155,7 +158,7 @@ func TestClaimPin_PostClaimPlanReadErrorFailsClosed(t *testing.T) { assertMocksOnCleanup(t, store, sender) manager := &Manager{config: store, email: sender, providerFactory: new(MockProviderFactory)} - didClaim, err := manager.claimAndExecute(ctx, &exec) + didClaim, err := manager.claimAndExecuteWithin(ctx, &exec, 0) require.True(t, didClaim) require.Error(t, err) assert.ErrorContains(t, err, "post-claim cap check") @@ -165,29 +168,6 @@ func TestClaimPin_PostClaimPlanReadErrorFailsClosed(t *testing.T) { store.AssertCalled(t, "DeleteSuppressionsByExecutionTx", ctx, mock.Anything, exec.ExecutionID) } -// Manual / approved purchases are unchanged: an approved row is claimed and run -// with no cap re-check, even on a plan that has no cap (the cap error class -// never appears and the plan is read once, by executePurchase itself). -func TestClaimPin_ApprovedRowSkipsCapRecheck(t *testing.T) { - ctx := context.Background() - exec := dueRecExec("approved", upfrontRec(1e9)) - store := new(MockConfigStore) - assertMocksOnCleanup(t, store) - store.On("GetPurchasePlan", mock.Anything, exec.PlanID).Return(autoPlan(nil), nil) - store.On("GetPlanAccounts", mock.Anything, exec.PlanID).Return(nil, errors.New("accounts unavailable")) - claimed := exec - claimed.Status = "running" - store.On("TransitionExecutionStatusPinned", ctx, exec.ExecutionID, mock.Anything, "running", (*string)(nil), false).Return(&claimed, nil) - store.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil) - manager := &Manager{config: store, email: allowFailedNotification(new(MockEmailSender)), providerFactory: new(MockProviderFactory)} - - didClaim, err := manager.claimAndExecute(ctx, &exec) - require.True(t, didClaim) - require.Error(t, err) - assert.NotErrorIs(t, err, ErrStepCommitmentCap) - store.AssertNumberOfCalls(t, "GetPurchasePlan", 1) -} - // S1: when the notification email fails nothing is promoted, and the scheduler // then refuses the still-pending rec-bearing row (no claim, no failure). func TestNotification_EmailFailureLeavesRowPendingAndSchedulerRefusesIt(t *testing.T) { diff --git a/internal/purchase/coverage_extra_test.go b/internal/purchase/coverage_extra_test.go index 67cb26c0b..05ea829b9 100644 --- a/internal/purchase/coverage_extra_test.go +++ b/internal/purchase/coverage_extra_test.go @@ -208,8 +208,8 @@ func TestResolveGCPProvider_ApplicationDefault(t *testing.T) { assert.Nil(t, result) } -// Tests for handleExecutePurchase: execution found + status "approved" → success path. -func TestHandleExecutePurchase_ApprovedStatus(t *testing.T) { +// Tests for the claim funnel: a claimed notified row runs to a successful purchase. +func TestClaimAndExecuteWithin_NotifiedRowExecutes(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) mockEmail := new(MockEmailSender) @@ -217,8 +217,10 @@ func TestHandleExecutePurchase_ApprovedStatus(t *testing.T) { mockSTS := new(MockSTSClient) plan := &config.PurchasePlan{ - ID: "plan-approved", - Name: "Approved Plan", + ID: "plan-approved", + Name: "Approved Plan", + AutoPurchase: true, + MaxStepCommitment: capOf(1e9), RampSchedule: config.RampSchedule{ TotalSteps: 4, CurrentStep: 0, @@ -228,16 +230,16 @@ func TestHandleExecutePurchase_ApprovedStatus(t *testing.T) { exec := &config.PurchaseExecution{ ExecutionID: "exec-approved", PlanID: "plan-approved", - Status: "approved", + Status: "notified", StepNumber: 1, Recommendations: []config.RecommendationRecord{{ Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", - Count: 1, Selected: true, + Count: 1, UpfrontCost: 100, Selected: true, }}, } - mockStore.On("GetExecutionByID", ctx, "exec-approved").Return(exec, nil) - // claimAndExecute CASes the row to "running" before executing (issue #1013). + mockStore.On("GetGlobalConfig", mock.Anything).Return(autoPurchaseOnCfg(), nil).Maybe() + // claimAndExecuteWithin CASes the row to "running" before executing (issue #1013). runningExec := *exec runningExec.Status = "running" mockStore.On("TransitionExecutionStatus", ctx, "exec-approved", @@ -264,42 +266,15 @@ func TestHandleExecutePurchase_ApprovedStatus(t *testing.T) { dashboardURL: "https://dashboard.example.com", } - msg := AsyncMessage{ - Type: MessageTypeExecutePurchase, - ExecutionID: "exec-approved", - } - err := manager.handleExecutePurchase(ctx, &msg) + claimed, err := manager.claimAndExecuteWithin(ctx, exec, 0) require.NoError(t, err) + require.True(t, claimed) mockStore.AssertExpectations(t) } -// Tests for handleExecutePurchase: GetExecutionByID returns error. -func TestHandleExecutePurchase_GetError(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - assertMocksOnCleanup(t, mockStore) - mockEmail := new(MockEmailSender) - - mockStore.On("GetExecutionByID", ctx, "exec-err").Return(nil, errors.New("db error")) - - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - - msg := AsyncMessage{ - Type: MessageTypeExecutePurchase, - ExecutionID: "exec-err", - } - err := manager.handleExecutePurchase(ctx, &msg) - assert.Error(t, err) - assert.Contains(t, err.Error(), "failed to get execution") -} - -// Tests for handleExecutePurchase: SavePurchaseExecution error after failed purchase. -func TestHandleExecutePurchase_SaveError(t *testing.T) { +// Tests for the claim funnel: SavePurchaseExecution error after failed purchase. +func TestClaimAndExecuteWithin_SaveError(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) mockEmail := allowFailedNotification(new(MockEmailSender)) @@ -331,10 +306,8 @@ func TestHandleExecutePurchase_SaveError(t *testing.T) { Recommendations: []config.RecommendationRecord{rec}, } - mockStore.On("GetGlobalConfig", ctx).Return(autoPurchaseOnCfg(), nil).Once() mockStore.On("GetGlobalConfig", mock.Anything).Return(autoPurchaseOnCfg(), nil) - mockStore.On("GetExecutionByID", ctx, "exec-save-err").Return(exec, nil) - // claimAndExecute CASes the row to "running" before executing (issue #1013). + // claimAndExecuteWithin CASes the row to "running" before executing (issue #1013). runningExec := *exec runningExec.Status = "running" mockStore.On("TransitionExecutionStatus", ctx, "exec-save-err", @@ -354,15 +327,12 @@ func TestHandleExecutePurchase_SaveError(t *testing.T) { dashboardURL: "https://dashboard.example.com", } - msg := AsyncMessage{ - Type: MessageTypeExecutePurchase, - ExecutionID: "exec-save-err", - } - err := manager.handleExecutePurchase(ctx, &msg) - // The terminal-save failure now surfaces from executeAndFinalize as - // ErrAuditLoss (the row is stranded in "running"). The SQS handler returns - // it so the message is redelivered. A single-account provider failure with - // no committed recs is not multi-account-ackable, so it is returned. + claimed, err := manager.claimAndExecuteWithin(ctx, exec, 0) + require.True(t, claimed) + // The terminal-save failure surfaces from executeAndFinalize as + // ErrAuditLoss (the row is stranded in "running"). A single-account + // provider failure with no committed recs is not multi-account-ackable, + // so it is returned. assert.Error(t, err) assert.ErrorIs(t, err, config.ErrAuditLoss) } diff --git a/internal/purchase/failure_notify_test.go b/internal/purchase/failure_notify_test.go index bfa6b9b75..eb4b0354d 100644 --- a/internal/purchase/failure_notify_test.go +++ b/internal/purchase/failure_notify_test.go @@ -6,6 +6,7 @@ import ( "fmt" "sync" "testing" + "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" @@ -148,7 +149,7 @@ func TestIsFanOutOutcome(t *testing.T) { assert.False(t, isFanOutOutcome(fmt.Errorf("%w: wrapped", config.ErrAuditLoss))) } -// End to end through the SQS executor: a multi-account run where one account +// End to end through the scheduled sweep: a multi-account run where one account // buys and one fails sends exactly one purchase-failed email, for the failed // account's row, and none for the aggregate root row. func TestMultiAccountFailureNotifiesOncePerFailedAccountNotRoot(t *testing.T) { @@ -172,7 +173,8 @@ func TestMultiAccountFailureNotifiesOncePerFailedAccountNotRoot(t *testing.T) { plan := &config.PurchasePlan{ID: "plan-x", Name: "Plan X", AutoPurchase: true, MaxStepCommitment: capOf(1e9)} store.On("GetGlobalConfig", mock.Anything).Return(failureNotifyCfg(), nil) - store.On("GetExecutionByID", ctx, "root-714").Return(exec, nil) + exec.ScheduledDate = time.Now().Add(-time.Minute) + expectSweepOf(ctx, store, *exec) running := *exec running.Status = "running" store.On("TransitionExecutionStatus", ctx, "root-714", @@ -201,7 +203,10 @@ func TestMultiAccountFailureNotifiesOncePerFailedAccountNotRoot(t *testing.T) { Return(common.PurchaseResult{Success: true, CommitmentID: "ri-ok"}, nil).Once() m := &Manager{config: store, email: sender, providerFactory: factory, credStore: awsAccessKeyCredStore()} - require.NoError(t, m.handleExecutePurchase(ctx, &AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: "root-714"})) + result, err := m.ProcessScheduledPurchases(ctx) + require.NoError(t, err) + require.Equal(t, 1, result.Executed, "a partial multi-account run counts as executed") + require.Equal(t, 0, result.Failed) assert.Equal(t, "partially_completed", rootStatus) require.Len(t, failedRows, 1) diff --git a/internal/purchase/failure_notify_writers_test.go b/internal/purchase/failure_notify_writers_test.go index 7f0d4c102..b0b01af6e 100644 --- a/internal/purchase/failure_notify_writers_test.go +++ b/internal/purchase/failure_notify_writers_test.go @@ -8,6 +8,7 @@ import ( "strings" "sync" "testing" + "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" @@ -133,7 +134,8 @@ func fanOutRun(t *testing.T, other config.CloudAccount, loadRaw func(string) ([] plan := &config.PurchasePlan{ID: "plan-x", Name: "Plan X", AutoPurchase: true, MaxStepCommitment: capOf(1e9)} store.On("GetGlobalConfig", mock.Anything).Return(failureNotifyCfg(), nil) - store.On("GetExecutionByID", ctx, "root-1195").Return(exec, nil) + exec.ScheduledDate = time.Now().Add(-time.Minute) + expectSweepOf(ctx, store, *exec) running := *exec running.Status = "running" store.On("TransitionExecutionStatus", ctx, "root-1195", @@ -167,7 +169,10 @@ func fanOutRun(t *testing.T, other config.CloudAccount, loadRaw func(string) ([] }} m := &Manager{config: store, email: sender, providerFactory: factory, credStore: creds} - require.NoError(t, m.handleExecutePurchase(ctx, &AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: "root-1195"})) + result, err := m.ProcessScheduledPurchases(ctx) + require.NoError(t, err) + require.Equal(t, 1, result.Executed, "a partial multi-account run counts as executed") + require.Equal(t, 0, result.Failed) mu.Lock() defer mu.Unlock() diff --git a/internal/purchase/money_path_regression_test.go b/internal/purchase/money_path_regression_test.go index bcf81ea15..9011555d9 100644 --- a/internal/purchase/money_path_regression_test.go +++ b/internal/purchase/money_path_regression_test.go @@ -6,6 +6,7 @@ import ( "sort" "sync" "testing" + "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" @@ -298,8 +299,8 @@ func TestMultiAccountSeedsStablePerAccountKey(t *testing.T) { assert.Equal(t, run1, run2, "two runs of the same root must reproduce identical per-account idempotency keys (issue #1012/H1)") } -// TestSQSRedeliveryDoesNotDoubleExecute is the issue #1013 (C2) regression -// guard: a redelivered execute_purchase SQS message must NOT execute the row a +// TestScheduledSweepOverlapDoesNotDoubleExecute is the issue #1013 (C2) regression +// guard: overlapping scheduled sweeps must NOT execute the same row a // second time. The first delivery wins the CAS to "running"; the redelivery // loses the CAS (the row is no longer in an executable state) and is acked // without touching the cloud. @@ -307,7 +308,7 @@ func TestMultiAccountSeedsStablePerAccountKey(t *testing.T) { // Pre-fix, handleExecutePurchase only checked status in (approved,pending) with // no atomic transition, so both deliveries would execute. This test asserts the // purchase runs exactly once and the second delivery is a benign no-op. -func TestSQSRedeliveryDoesNotDoubleExecute(t *testing.T) { +func TestScheduledSweepOverlapDoesNotDoubleExecute(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) mockEmail := new(MockEmailSender) @@ -326,6 +327,7 @@ func TestSQSRedeliveryDoesNotDoubleExecute(t *testing.T) { PlanID: "plan-dup", // a plan-less row is never auto-executed IdempotencyKey: "lineage-dup", Status: "notified", + ScheduledDate: time.Now().Add(-time.Minute), Recommendations: []config.RecommendationRecord{ {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Selected: true}, }, @@ -338,8 +340,8 @@ func TestSQSRedeliveryDoesNotDoubleExecute(t *testing.T) { // pending copy, faithfully modeling the real double-delivery scenario. mockStore.On("GetGlobalConfig", ctx).Return(autoPurchaseOnCfg(), nil).Once() mockStore.On("GetGlobalConfig", mock.Anything).Return(autoPurchaseOnCfg(), nil).Maybe() - mockStore.On("GetExecutionByID", ctx, "exec-dup").Return(newPending(), nil).Once() - mockStore.On("GetExecutionByID", ctx, "exec-dup").Return(newPending(), nil).Once() + mockStore.On("GetStaleApprovedExecutions", ctx, mock.Anything).Return([]config.PurchaseExecution{}, nil) + mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{*newPending()}, nil).Twice() // First claim wins: pending -> running, returns the running row. The second // claim (redelivery) loses with ErrExecutionNotInExpectedStatus, because the @@ -374,9 +376,13 @@ func TestSQSRedeliveryDoesNotDoubleExecute(t *testing.T) { dashboardURL: "https://dashboard.example.com", } - msg := AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: "exec-dup"} - require.NoError(t, manager.handleExecutePurchase(ctx, &msg), "first delivery executes cleanly") - require.NoError(t, manager.handleExecutePurchase(ctx, &msg), "redelivery is a benign no-op (acked)") + first, err := manager.ProcessScheduledPurchases(ctx) + require.NoError(t, err) + assert.Equal(t, 1, first.Executed, "first sweep executes cleanly") + second, err := manager.ProcessScheduledPurchases(ctx) + require.NoError(t, err) + assert.Equal(t, 0, second.Executed, "the overlapping sweep loses the claim and is a benign no-op") + assert.Equal(t, 0, second.Failed) mu.Lock() defer mu.Unlock() @@ -422,7 +428,8 @@ func TestMultiAccountPartialSuccessIsAcked(t *testing.T) { mockStore.On("GetGlobalConfig", ctx).Return(autoPurchaseOnCfg(), nil).Once() mockStore.On("GetGlobalConfig", mock.Anything).Return(autoPurchaseOnCfg(), nil).Maybe() - mockStore.On("GetExecutionByID", ctx, "root-partial").Return(exec, nil) + exec.ScheduledDate = time.Now().Add(-time.Minute) + expectSweepOf(ctx, mockStore, *exec) running := *exec running.Status = "running" mockStore.On("TransitionExecutionStatus", ctx, "root-partial", @@ -464,10 +471,11 @@ func TestMultiAccountPartialSuccessIsAcked(t *testing.T) { dashboardURL: "https://dashboard.example.com", } - msg := AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: "root-partial"} - err := manager.handleExecutePurchase(ctx, &msg) - require.NoError(t, err, - "a multi-account run with >=1 committed account must be ACKed (return nil), not redelivered (issue #1014)") + result, err := manager.ProcessScheduledPurchases(ctx) + require.NoError(t, err) + assert.Equal(t, 1, result.Executed, + "a multi-account run with >=1 committed account must be ACKed (counted Executed), not a flat failure (issue #1014)") + assert.Equal(t, 0, result.Failed) require.NotNil(t, savedRoot, "the root row must be saved with its aggregate status (H2)") assert.Equal(t, "partially_completed", savedRoot.Status, @@ -527,7 +535,8 @@ func TestExecuteForAccount_CommittedButUnsavedIsAcked(t *testing.T) { mockStore.On("GetGlobalConfig", ctx).Return(autoPurchaseOnCfg(), nil).Once() mockStore.On("GetGlobalConfig", mock.Anything).Return(autoPurchaseOnCfg(), nil).Maybe() - mockStore.On("GetExecutionByID", ctx, "root-unsaved").Return(exec, nil) + exec.ScheduledDate = time.Now().Add(-time.Minute) + expectSweepOf(ctx, mockStore, *exec) running := *exec running.Status = "running" mockStore.On("TransitionExecutionStatus", ctx, "root-unsaved", @@ -575,10 +584,11 @@ func TestExecuteForAccount_CommittedButUnsavedIsAcked(t *testing.T) { dashboardURL: "https://dashboard.example.com", } - msg := AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: "root-unsaved"} - err := manager.handleExecutePurchase(ctx, &msg) - require.NoError(t, err, - "a committed-but-unsaved account must still ACK (return nil), never redeliver and double-buy (#258/A16-006)") + result, err := manager.ProcessScheduledPurchases(ctx) + require.NoError(t, err) + require.Equal(t, 1, result.Executed, + "a committed-but-unsaved account must still count as executed, never retried and double-bought (#258/A16-006)") + assert.Equal(t, 0, result.Failed) require.NotNil(t, savedRoot, "the root row must be saved with its aggregate status") assert.Equal(t, "partially_completed", savedRoot.Status, diff --git a/internal/purchase/step_cap_test.go b/internal/purchase/step_cap_test.go index 11412c7dd..f942166e4 100644 --- a/internal/purchase/step_cap_test.go +++ b/internal/purchase/step_cap_test.go @@ -173,59 +173,6 @@ func TestScheduler_RefusesBeforeClaim(t *testing.T) { } } -// The SQS execute_purchase path enforces the same refusals before the claim. -func TestSQSExecutePath_RefusesBeforeClaim(t *testing.T) { - capped := func(c *float64) *config.PurchasePlan { - return &config.PurchasePlan{ID: "plan-gate", AutoPurchase: true, MaxStepCommitment: c} - } - cases := []struct { - name string - plan *config.PurchasePlan - exec config.PurchaseExecution - cfg *config.GlobalConfig - cfgErr error - }{ - {"kill switch off", capped(f64(1000)), dueRecExec("notified", upfrontRec(10)), &config.GlobalConfig{}, nil}, - {"kill switch read error", capped(f64(1000)), dueRecExec("notified", upfrontRec(10)), nil, errors.New("db down")}, - {"pending rec-bearing row", capped(f64(1000)), dueRecExec("pending", upfrontRec(10)), autoPurchaseOnCfg(), nil}, - {"NULL cap", capped(nil), dueRecExec("notified", upfrontRec(10)), autoPurchaseOnCfg(), nil}, - {"over cap", capped(f64(5)), dueRecExec("notified", upfrontRec(10)), autoPurchaseOnCfg(), nil}, - {"zero commitment", capped(f64(1000)), dueRecExec("notified", upfrontRec(0)), autoPurchaseOnCfg(), nil}, - {"plan missing", nil, dueRecExec("notified", upfrontRec(10)), autoPurchaseOnCfg(), nil}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - ctx := context.Background() - store := new(MockConfigStore) - assertMocksOnCleanup(t, store) - exec := tc.exec - store.On("GetExecutionByID", ctx, exec.ExecutionID).Return(&exec, nil) - store.On("GetPurchasePlan", ctx, exec.PlanID).Return(tc.plan, nil) - expectKillSwitchRead(ctx, store, tc.plan, tc.cfg, tc.cfgErr) - manager := &Manager{config: store, email: new(MockEmailSender)} - - err := manager.handleExecutePurchase(ctx, &AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: exec.ExecutionID}) - - require.Error(t, err, "the message must be dead-lettered, not acked as done") - store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) - }) - } -} - -// Manual purchases are not touched by the kill switch or the cap: an approved -// row (human approval path) bypasses the auto-execute gate entirely, and the -// gate never reads the global config for it. -func TestAutoExecuteGate_ApprovedRowIgnoresKillSwitchAndCap(t *testing.T) { - ctx := context.Background() - store := new(MockConfigStore) // no GetGlobalConfig / GetPurchasePlan expectation: any read panics - assertMocksOnCleanup(t, store) - manager := &Manager{config: store, email: new(MockEmailSender)} - exec := dueRecExec("approved", upfrontRec(1e9)) - require.NoError(t, manager.checkAutoExecuteGate(ctx, &exec)) - store.AssertNotCalled(t, "GetGlobalConfig", mock.Anything) - store.AssertNotCalled(t, "GetPurchasePlan", mock.Anything, mock.Anything) -} - // A web-submitted direct purchase (no plan) never reaches the plan gate. func TestExecutableByScheduler_WebRowNeverReadsKillSwitch(t *testing.T) { store := new(MockConfigStore) @@ -312,3 +259,10 @@ func TestScheduler_DirectExecuteRowSkippedWithoutPlanLookup(t *testing.T) { store.AssertNotCalled(t, "GetPurchasePlan", mock.Anything, mock.Anything) store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } + +// expectSweepOf registers the reads ProcessScheduledPurchases makes before it +// gates and claims the given due rows. +func expectSweepOf(ctx context.Context, store *MockConfigStore, rows ...config.PurchaseExecution) { + store.On("GetStaleApprovedExecutions", ctx, mock.Anything).Return([]config.PurchaseExecution{}, nil) + store.On("GetPendingExecutions", ctx).Return(rows, nil) +} From c490855a0c447e543c51a752724f36545931d8cf Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 15:57:35 +0100 Subject: [PATCH 04/14] test(purchase): run the redrive and ladder guards through the claim funnel The armed Azure savings plan, AWS no-token and ladder recheck tests call claimAndExecuteWithin on notified rows instead of an SQS message. The shared harness plan now carries a step cap so the post-claim cap recheck passes and the guards under test, not the cap, decide the outcome. Refs #399 Co-Authored-By: Claude Opus 5.5 --- internal/purchase/armed_redrive_test.go | 38 ++++++++----------- .../purchase/aws_no_token_redrive_test.go | 19 ++++------ .../purchase/ladder_execution_recheck_test.go | 26 ++++++------- 3 files changed, 35 insertions(+), 48 deletions(-) diff --git a/internal/purchase/armed_redrive_test.go b/internal/purchase/armed_redrive_test.go index ce7fbdf24..c1455b6d8 100644 --- a/internal/purchase/armed_redrive_test.go +++ b/internal/purchase/armed_redrive_test.go @@ -2,7 +2,6 @@ package purchase import ( "context" - "encoding/json" "sync" "testing" "time" @@ -101,7 +100,7 @@ func armedHarness(t *testing.T, serviceType common.ServiceType) (*Manager, *Mock // precisely what is under test. store.SavePurchaseExecutionFn = func(_ context.Context, _ *config.PurchaseExecution) error { return nil } store.GetPurchasePlanFn = func(_ context.Context, id string) (*config.PurchasePlan, error) { - return &config.PurchasePlan{ID: id, Name: "Plan 1718"}, nil + return &config.PurchasePlan{ID: id, Name: "Plan 1718", AutoPurchase: true, MaxStepCommitment: capOf(1e9)}, nil } store.GetPlanAccountsFn = func(_ context.Context, _ string) ([]config.CloudAccount, error) { return nil, nil } @@ -160,20 +159,18 @@ func TestApproveAndExecuteRefusesArmedAzureSavingsPlanRetry(t *testing.T) { assert.Contains(t, err.Error(), "second savings plan") } -// TestClaimAndExecuteRefusesArmedAzureSavingsPlanRetry covers the SQS -// execute_purchase and cron sweep path into the same armed row. +// TestClaimAndExecuteRefusesArmedAzureSavingsPlanRetry covers the cron sweep's +// claim funnel into the same armed row. The fixture row is web-sourced, which +// the sweep's own gate would skip, so the funnel is driven directly. func TestClaimAndExecuteRefusesArmedAzureSavingsPlanRetry(t *testing.T) { - exec := armedExecution("azure", "savingsplans", 2, "approved") + exec := armedExecution("azure", "savingsplans", 2, "notified") mgr, store, rec := armedHarness(t, common.ServiceSavingsPlansAll) - store.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil).Maybe() expectClaim(store, exec, []string{"approved", "pending", "notified"}) - body, err := json.Marshal(AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: exec.ExecutionID}) - require.NoError(t, err) - _ = mgr.ProcessMessage(context.Background(), string(body)) + _, _ = mgr.claimAndExecuteWithin(context.Background(), exec, 0) assert.Equal(t, 0, rec.count(), - "the SQS/cron executor must not buy a second savings plan for an armed retry successor (issue #1718)") + "the cron claim funnel must not buy a second savings plan for an armed retry successor (issue #1718)") } // TestFireScheduledDelayedPurchasesRefusesArmedAzureSavingsPlanRetry covers the @@ -208,16 +205,15 @@ func TestFireScheduledDelayedPurchasesRefusesArmedAzureSavingsPlanRetry(t *testi // block every legitimate first savings-plan purchase and still pass every test // above. func TestFirstAzureSavingsPlanPurchaseStillExecutes(t *testing.T) { - exec := armedExecution("azure", "savingsplans", 0, "approved") + exec := armedExecution("azure", "savingsplans", 0, "notified") exec.Recommendations[0].ID = "current-azure" mgr, store, rec := armedHarness(t, common.ServiceSavingsPlansAll) store.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}).Return(exec.Recommendations, nil).Once() - store.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil).Maybe() expectClaim(store, exec, []string{"approved", "pending", "notified"}) - body, err := json.Marshal(AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: exec.ExecutionID}) + claimed, err := mgr.claimAndExecuteWithin(context.Background(), exec, 0) require.NoError(t, err) - require.NoError(t, mgr.ProcessMessage(context.Background(), string(body))) + require.True(t, claimed) assert.Equal(t, 1, rec.count(), "a first Azure savings-plans purchase (retry_attempt_n = 0) must still go through; blocking it would be a worse bug than the one being fixed") @@ -229,14 +225,13 @@ func TestFirstAzureSavingsPlanPurchaseStillExecutes(t *testing.T) { // re-drive onto the original and the retry must still execute. EC2 and // Redshift are the AWS services that lack such a guard (see the Refuses tests). func TestSafeProviderRetrySuccessorStillExecutes(t *testing.T) { - exec := armedExecution("aws", "rds", 3, "approved") + exec := armedExecution("aws", "rds", 3, "notified") mgr, store, rec := armedHarness(t, common.ServiceRDS) - store.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil).Maybe() expectClaim(store, exec, []string{"approved", "pending", "notified"}) - body, err := json.Marshal(AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: exec.ExecutionID}) + claimed, err := mgr.claimAndExecuteWithin(context.Background(), exec, 0) require.NoError(t, err) - require.NoError(t, mgr.ProcessMessage(context.Background(), string(body))) + require.True(t, claimed) assert.Equal(t, 1, rec.count(), "an AWS RDS retry successor dedupes at the provider via its reservation ID and must still execute; the guard must not widen beyond the providers that lack a duplicate guard") @@ -248,7 +243,7 @@ func TestSafeProviderRetrySuccessorStillExecutes(t *testing.T) { // "failed" with the reason and executeAndFinalize persists it. Once failed, the // #1668 creation-time gate refuses to retry it, so it cannot be re-armed. func TestArmedRowIsDefusedNotLeftArmed(t *testing.T) { - exec := armedExecution("azure", "savingsplans", 1, "approved") + exec := armedExecution("azure", "savingsplans", 1, "notified") mgr, store, rec := armedHarness(t, common.ServiceSavingsPlansAll) var saved []config.PurchaseExecution @@ -256,12 +251,9 @@ func TestArmedRowIsDefusedNotLeftArmed(t *testing.T) { saved = append(saved, *e) return nil } - store.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil).Maybe() expectClaim(store, exec, []string{"approved", "pending", "notified"}) - body, err := json.Marshal(AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: exec.ExecutionID}) - require.NoError(t, err) - _ = mgr.ProcessMessage(context.Background(), string(body)) + _, _ = mgr.claimAndExecuteWithin(context.Background(), exec, 0) require.Equal(t, 0, rec.count()) require.NotEmpty(t, saved, "the refused row must be persisted, not left in its claimed status") diff --git a/internal/purchase/aws_no_token_redrive_test.go b/internal/purchase/aws_no_token_redrive_test.go index f988db20d..2497716bf 100644 --- a/internal/purchase/aws_no_token_redrive_test.go +++ b/internal/purchase/aws_no_token_redrive_test.go @@ -2,7 +2,6 @@ package purchase import ( "context" - "encoding/json" "testing" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -95,18 +94,15 @@ func TestAWSNoTokenRetrySuccessorIsRefusedAndFirstBuyStillExecutes(t *testing.T) t.Run(service, func(t *testing.T) { svcType := m.mapServiceType(service) - retry := armedExecution("aws", service, 2, "approved") + retry := armedExecution("aws", service, 2, "notified") mgr, store, rec := armedHarness(t, svcType) var saved []config.PurchaseExecution store.SavePurchaseExecutionFn = func(_ context.Context, e *config.PurchaseExecution) error { saved = append(saved, *e) return nil } - store.On("GetExecutionByID", mock.Anything, retry.ExecutionID).Return(retry, nil).Maybe() expectClaim(store, retry, []string{"approved", "pending", "notified"}) - body, err := json.Marshal(AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: retry.ExecutionID}) - require.NoError(t, err) - _ = mgr.ProcessMessage(context.Background(), string(body)) + _, _ = mgr.claimAndExecuteWithin(context.Background(), retry, 0) assert.Equal(t, 0, rec.count(), "a retry after a possibly-landed %s purchase must not buy again", service) require.NotEmpty(t, saved) @@ -114,11 +110,12 @@ func TestAWSNoTokenRetrySuccessorIsRefusedAndFirstBuyStillExecutes(t *testing.T) assert.Equal(t, "failed", final.Status) assert.Contains(t, final.Error, "Check EC2 Reserved Instances or Redshift Reserved Nodes") - first := armedExecution("aws", service, 0, "approved") + first := armedExecution("aws", service, 0, "notified") mgr, store, rec = armedHarness(t, svcType) - store.On("GetExecutionByID", mock.Anything, first.ExecutionID).Return(first, nil).Maybe() expectClaim(store, first, []string{"approved", "pending", "notified"}) - require.NoError(t, mgr.ProcessMessage(context.Background(), string(body))) + claimed, err := mgr.claimAndExecuteWithin(context.Background(), first, 0) + require.NoError(t, err) + require.True(t, claimed) assert.Equal(t, 1, rec.count(), "a first %s purchase must still go through; the guard is for retries only", service) }) } @@ -162,8 +159,8 @@ func TestRecoverStrandedApprovals_EC2RowIsFailedNotRedriven(t *testing.T) { } // TestApproveAndExecuteRefusesAWSNoTokenRetrySuccessor pins the refusal on the -// approval entry points (issue #780). The ProcessMessage test above covers the -// SQS/cron executor; this drives the two ways an operator approves a pending +// approval entry points (issue #780). The claim-funnel test above covers the +// cron claim funnel; this drives the two ways an operator approves a pending // row: the email-link token path (ApproveExecution, which calls // ApproveAndExecute with no session UUID) and the session path // (ApproveAndExecute with the session user's UUID as transitionedBy). diff --git a/internal/purchase/ladder_execution_recheck_test.go b/internal/purchase/ladder_execution_recheck_test.go index c523cfe7c..8a640b6e2 100644 --- a/internal/purchase/ladder_execution_recheck_test.go +++ b/internal/purchase/ladder_execution_recheck_test.go @@ -2,7 +2,6 @@ package purchase import ( "context" - "encoding/json" "errors" "testing" @@ -37,7 +36,7 @@ func ladderGlobal(planning, execution bool) *config.GlobalConfig { // replaced by the case under test. func ladderHarness(t *testing.T, key string, gate *config.LadderExecutionGate, gateErr error, global *config.GlobalConfig, globalErr error) (*Manager, *MockConfigStore, *armedPurchaseRecorder, *config.PurchaseExecution) { t.Helper() - exec := armedExecution("aws", "savingsplans", 0, "approved") + exec := armedExecution("aws", "savingsplans", 0, "notified") exec.IdempotencyKey = key mgr, store, rec := armedHarness(t, common.ServiceSavingsPlansAll) kept := store.ExpectedCalls[:0] @@ -49,16 +48,15 @@ func ladderHarness(t *testing.T, key string, gate *config.LadderExecutionGate, g store.ExpectedCalls = kept store.On("GetGlobalConfig", mock.Anything).Return(global, globalErr).Maybe() store.On("GetLadderExecutionGate", mock.Anything, key).Return(gate, gateErr).Maybe() - store.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil).Maybe() return mgr, store, rec, exec } -func runViaSQS(t *testing.T, mgr *Manager, store *MockConfigStore, exec *config.PurchaseExecution) { +// runViaClaimFunnel runs exec through the claim funnel the cron sweep calls +// after its gate, with the from-set that production passes. +func runViaClaimFunnel(t *testing.T, mgr *Manager, store *MockConfigStore, exec *config.PurchaseExecution) { t.Helper() expectClaim(store, exec, []string{"approved", "pending", "notified"}) - body, err := json.Marshal(AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: exec.ExecutionID}) - require.NoError(t, err) - _ = mgr.ProcessMessage(context.Background(), string(body)) + _, _ = mgr.claimAndExecuteWithin(context.Background(), exec, 0) } // TestLadderRecheck_RefusesBeforeProviderCall covers every way the switches can @@ -102,7 +100,7 @@ func TestLadderRecheck_RefusesBeforeProviderCall(t *testing.T) { saved = append(saved, *e) return nil } - runViaSQS(t, mgr, store, exec) + runViaClaimFunnel(t, mgr, store, exec) assert.Equal(t, 0, rec.count(), "a refused ladder execution must reach no provider") require.NotEmpty(t, saved) @@ -120,7 +118,7 @@ func TestLadderRecheck_AllowsWhenEverythingIsOn(t *testing.T) { gate := openGate() gate.TrancheStatus = ptr(string(status)) mgr, store, rec, exec := ladderHarness(t, ladderTrancheKey, gate, nil, ladderGlobal(true, true), nil) - runViaSQS(t, mgr, store, exec) + runViaClaimFunnel(t, mgr, store, exec) assert.Equal(t, 1, rec.count(), "an armed ladder execution on a %s tranche must still buy exactly once", status) }) } @@ -137,15 +135,15 @@ func TestLadderRecheck_NonLadderKeysUnaffected(t *testing.T) { } { t.Run(name, func(t *testing.T) { mgr, store, rec, exec := ladderHarness(t, key, nil, nil, ladderGlobal(false, false), nil) - runViaSQS(t, mgr, store, exec) + runViaClaimFunnel(t, mgr, store, exec) assert.Equal(t, 1, rec.count(), "a non-ladder purchase must not be gated by the ladder switches") }) } } // Every executor entry point reaches executeAndFinalize, so the recheck must -// stop each one. ProcessScheduledPurchases and the SQS message both land in -// claimAndExecute, the first case below. +// stop each one. ProcessScheduledPurchases lands in claimAndExecuteWithin, +// the first case below. func TestLadderRecheck_EveryExecutorEntryRefuses(t *testing.T) { off := ladderGlobal(true, false) t.Run("approve", func(t *testing.T) { @@ -157,9 +155,9 @@ func TestLadderRecheck_EveryExecutorEntryRefuses(t *testing.T) { require.Error(t, err) assert.Equal(t, 0, rec.count()) }) - t.Run("claimAndExecute (SQS and cron sweep)", func(t *testing.T) { + t.Run("claimAndExecute (cron sweep funnel)", func(t *testing.T) { mgr, store, rec, exec := ladderHarness(t, ladderTrancheKey, openGate(), nil, off, nil) - runViaSQS(t, mgr, store, exec) + runViaClaimFunnel(t, mgr, store, exec) assert.Equal(t, 0, rec.count()) }) t.Run("scheduler fire", func(t *testing.T) { From 2acda0c9c962437d4cd59a71229609481ddb6b56 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 15:57:43 +0100 Subject: [PATCH 05/14] test(api): execute retry successors through ApproveAndExecute The retry redrive and fan-out suites ran the persisted successor through an SQS execute message. They now keep the pending status the retry handler persists and approve it with ApproveAndExecute, the production path, with the same purchase-token assertions. Refs #399 Co-Authored-By: Claude Opus 5.5 --- .../handler_purchases_retry_fanout_test.go | 30 ++++++++---------- .../handler_purchases_retry_redrive_test.go | 31 ++++++++----------- 2 files changed, 26 insertions(+), 35 deletions(-) diff --git a/internal/api/handler_purchases_retry_fanout_test.go b/internal/api/handler_purchases_retry_fanout_test.go index 829a1317d..5b03388c1 100644 --- a/internal/api/handler_purchases_retry_fanout_test.go +++ b/internal/api/handler_purchases_retry_fanout_test.go @@ -2,7 +2,6 @@ package api import ( "context" - "encoding/json" "sort" "sync" "testing" @@ -167,27 +166,27 @@ func (f *fanoutProviderFactory) CreateAndValidateProvider(_ context.Context, _ s } // executeSuccessorForReal runs the retry successor through the REAL -// purchase.Manager the way production does: the approved row is picked up from -// the execute_purchase queue message, claimed, and executed. It returns the +// purchase.Manager the way production does: the pending row is approved by +// an operator, claimed, and executed. It returns the // sorted idempotency tokens of every commitment purchase that reached the // cloud — one entry per real purchase. func executeSuccessorForReal(t *testing.T, successor *config.PurchaseExecution) []string { t.Helper() ctx := context.Background() - // The row as the DB holds it once the operator has clicked the approval - // link: same fields the retry handler persisted, status "approved". - approved := *successor - approved.Status = "approved" - running := approved + // The row as the retry handler persisted it (status pending), awaiting the + // operator's approval. + pending := *successor + running := pending running.Status = "running" store := new(MockConfigStore) t.Cleanup(func() { store.AssertExpectations(t) }) - store.On("GetExecutionByID", mock.Anything, approved.ExecutionID).Return(&approved, nil).Once() - store.On("TransitionExecutionStatus", mock.Anything, approved.ExecutionID, - []string{"approved", "pending", "notified"}, "running", (*string)(nil)).Return(&running, nil).Once() + store.On("GetExecutionByID", mock.Anything, pending.ExecutionID).Return(&pending, nil).Maybe() + store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + store.On("TransitionExecutionStatus", mock.Anything, pending.ExecutionID, + []string{"pending", "notified"}, "running", (*string)(nil)).Return(&running, nil).Once() store.On("SavePurchaseHistory", mock.Anything, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) // Plan progress advances only on a fully clean run; .Maybe() so a run that // ends partial/failed does not turn into a mock-expectation failure that @@ -223,12 +222,9 @@ func executeSuccessorForReal(t *testing.T, successor *config.PurchaseExecution) DashboardURL: "https://dashboard.example.com", }) - body, err := json.Marshal(purchase.AsyncMessage{ - Type: purchase.MessageTypeExecutePurchase, - ExecutionID: approved.ExecutionID, - }) - require.NoError(t, err) - require.NoError(t, mgr.ProcessMessage(ctx, string(body)), + // Human approval of the persisted pending successor, as the retry flow does. + _, _, err := mgr.ApproveAndExecute(ctx, pending.ExecutionID, "", nil, nil) + require.NoError(t, err, "the successor must execute cleanly; a non-nil error would make the purchase tally meaningless") return svc.purchasedTokens() diff --git a/internal/api/handler_purchases_retry_redrive_test.go b/internal/api/handler_purchases_retry_redrive_test.go index 4d9736128..dbc3b808f 100644 --- a/internal/api/handler_purchases_retry_redrive_test.go +++ b/internal/api/handler_purchases_retry_redrive_test.go @@ -2,7 +2,6 @@ package api import ( "context" - "encoding/json" "testing" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -113,30 +112,29 @@ func purchasesFiredByRetry(t *testing.T, failed *config.PurchaseExecution, req * } // executeRedriveSuccessor runs a retry successor through the real -// purchase.Manager the way production does: the row is picked up from the -// execute_purchase queue message once the operator has approved it, claimed, -// and executed. Returns the idempotency token of every purchase that reached +// purchase.Manager the way production does: the persisted pending row is +// approved by an operator, claimed, and executed. Returns the idempotency token of every purchase that reached // the cloud. func executeRedriveSuccessor(t *testing.T, successor *config.PurchaseExecution) []string { t.Helper() - approved := *successor - approved.Status = "approved" - running := approved + pending := *successor + running := pending running.Status = "running" store := new(MockConfigStore) - for _, rec := range approved.Recommendations { + for _, rec := range pending.Recommendations { if rec.Provider == "azure" { - store.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}).Return(approved.Recommendations, nil).Once() + store.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}).Return(pending.Recommendations, nil).Once() break } } t.Cleanup(func() { store.AssertExpectations(t) }) - store.On("GetExecutionByID", mock.Anything, approved.ExecutionID).Return(&approved, nil).Once() - store.On("TransitionExecutionStatus", mock.Anything, approved.ExecutionID, - []string{"approved", "pending", "notified"}, "running", (*string)(nil)).Return(&running, nil).Once() + store.On("GetExecutionByID", mock.Anything, pending.ExecutionID).Return(&pending, nil).Maybe() + store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + store.On("TransitionExecutionStatus", mock.Anything, pending.ExecutionID, + []string{"pending", "notified"}, "running", (*string)(nil)).Return(&running, nil).Once() store.On("SavePurchaseHistory", mock.Anything, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) store.SavePurchaseExecutionFn = func(_ context.Context, _ *config.PurchaseExecution) error { return nil } @@ -149,12 +147,9 @@ func executeRedriveSuccessor(t *testing.T, successor *config.PurchaseExecution) DashboardURL: "https://dashboard.example.com", }) - body, err := json.Marshal(purchase.AsyncMessage{ - Type: purchase.MessageTypeExecutePurchase, - ExecutionID: approved.ExecutionID, - }) - require.NoError(t, err) - require.NoError(t, mgr.ProcessMessage(context.Background(), string(body)), + // Human approval of the persisted pending successor, as the retry flow does. + _, _, err := mgr.ApproveAndExecute(context.Background(), pending.ExecutionID, "", nil, nil) + require.NoError(t, err, "the successor must execute cleanly; a non-nil error would make the purchase tally meaningless") return svc.purchasedTokens() From ddbd504aac9c36865764f1007f66e63c4414e62a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 15:57:53 +0100 Subject: [PATCH 06/14] test: run the integration suites through the sweep and approval paths The pause/claim worker is ProcessScheduledPurchases (a pending row is now skipped silently instead of refused), ramp-step root rows run through the sweep, per-account retry rows are pending and approved with ApproveAndExecute, and the claim-race and retention tests call claimAndExecuteWithin and claimAndRedrive. Refs #399 Co-Authored-By: Claude Opus 5.5 --- ..._purchases_pause_claim_integration_test.go | 54 +++++++++++------- .../plan_step_retention_integration_test.go | 6 +- .../ramp_step_progress_integration_test.go | 55 ++++++++++++------- .../purchase/single_claim_integration_test.go | 18 +++--- 4 files changed, 84 insertions(+), 49 deletions(-) diff --git a/internal/api/handler_purchases_pause_claim_integration_test.go b/internal/api/handler_purchases_pause_claim_integration_test.go index 4c8ba032a..e19672eb5 100644 --- a/internal/api/handler_purchases_pause_claim_integration_test.go +++ b/internal/api/handler_purchases_pause_claim_integration_test.go @@ -4,7 +4,6 @@ package api import ( "context" - "encoding/json" "sync" "sync/atomic" "testing" @@ -63,7 +62,6 @@ type pauseClaimFixture struct { manager *purchase.Manager execution *config.PurchaseExecution request *events.LambdaFunctionURLRequest - body string workers sync.WaitGroup } @@ -71,12 +69,14 @@ type pauseClaimFixture struct { var pauseClaimCap = 1e9 // markNotified moves the row to "notified", the only state in which the -// scheduler and SQS paths will auto-execute a recommendation-bearing plan row -// (the notification email is the veto window). pause only applies to "pending", -// so tests call this after pausing and resuming and before the worker runs. +// scheduler sweep will auto-execute a recommendation-bearing plan row +// (the notification email is the veto window) and due. Resume pushes a past +// scheduled date forward, which the sweep honors, so this also makes the row +// due again. pause only applies to "pending", so tests call this after pausing +// and resuming and before the worker runs. func (f *pauseClaimFixture) markNotified(t *testing.T) { t.Helper() - _, err := f.pool.Exec(f.ctx, `UPDATE purchase_executions SET status = 'notified' WHERE execution_id = $1`, f.execution.ExecutionID) + _, err := f.pool.Exec(f.ctx, `UPDATE purchase_executions SET status = 'notified', scheduled_date = now() - interval '1 minute' WHERE execution_id = $1`, f.execution.ExecutionID) require.NoError(t, err) } @@ -133,9 +133,6 @@ func newPauseClaimFixture(t *testing.T) *pauseClaimFixture { require.Equal(t, f.execution.CloudAccountID, row.CloudAccountID) require.NotEqual(t, common.PurchaseSourceWeb, row.Source) f.request = &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}} - body, err := json.Marshal(purchase.AsyncMessage{Type: purchase.MessageTypeExecutePurchase, ExecutionID: row.ExecutionID}) - require.NoError(t, err) - f.body = string(body) t.Cleanup(func() { f.service.unblock(); cancel(); f.workers.Wait() }) return f } @@ -143,10 +140,22 @@ func newPauseClaimFixture(t *testing.T) *pauseClaimFixture { func (f *pauseClaimFixture) startWorker() <-chan error { done := make(chan error, 1) f.workers.Add(1) - go func() { defer f.workers.Done(); done <- f.manager.ProcessMessage(f.ctx, f.body) }() + go func() { + defer f.workers.Done() + _, err := f.manager.ProcessScheduledPurchases(f.ctx) + done <- err + }() return done } +// sweep runs one scheduler tick, the production worker for a due plan row. +func (f *pauseClaimFixture) sweep(t *testing.T) *purchase.ProcessResult { + t.Helper() + result, err := f.manager.ProcessScheduledPurchases(f.ctx) + require.NoError(t, err) + return result +} + func (f *pauseClaimFixture) waitWorker(t *testing.T, done <-chan error) { t.Helper() select { @@ -219,11 +228,11 @@ func TestPauseClaim_RunningWorkerCannotBeReclaimed(t *testing.T) { before := f.state(t) require.Equal(t, "running", before.Status) require.Nil(t, before.Actor) - require.NoError(t, f.manager.ProcessMessage(f.ctx, f.body)) + f.sweep(t) require.EqualValues(t, 1, f.service.calls.Load()) pauseErr, resumeErr := f.pause(), f.resume() // Reach the purchase counter even when the old handler incorrectly accepts both actions. - require.NoError(t, f.manager.ProcessMessage(f.ctx, f.body)) + f.sweep(t) assert.EqualValues(t, 1, f.service.calls.Load(), "pause/resume must not allow a second provider entry") assertPauseConflict(t, pauseErr) assertPauseConflict(t, resumeErr) @@ -240,19 +249,19 @@ func TestPauseClaim_PausedQueueResumesExactlyOnce(t *testing.T) { require.Equal(t, "paused", paused.Status) require.Equal(t, f.execution.CreatedByUserID, paused.Actor) require.NotNil(t, paused.TransitionedAt) - require.NoError(t, f.manager.ProcessMessage(f.ctx, f.body)) + f.sweep(t) require.Zero(t, f.service.calls.Load()) require.Equal(t, paused, f.state(t)) require.NoError(t, f.resume()) require.Equal(t, "pending", f.state(t).Status) - // A resumed row is pending again, so auto-execution still refuses it until - // a notification has gone out. - require.Error(t, f.manager.ProcessMessage(f.ctx, f.body)) + // A resumed row is pending again, so the sweep still skips it until a + // notification has gone out. + require.Zero(t, f.sweep(t).Executed) require.Zero(t, f.service.calls.Load()) f.markNotified(t) f.service.unblock() - require.NoError(t, f.manager.ProcessMessage(f.ctx, f.body)) - require.NoError(t, f.manager.ProcessMessage(f.ctx, f.body)) + f.sweep(t) + f.sweep(t) f.assertCompleted(t) } @@ -277,7 +286,12 @@ func TestPauseClaim_ConcurrentPauseAndWorker(t *testing.T) { worker := make(chan error, 1) f.workers.Add(2) go func() { defer f.workers.Done(); <-start; paused <- f.pause() }() - go func() { defer f.workers.Done(); <-start; worker <- f.manager.ProcessMessage(f.ctx, f.body) }() + go func() { + defer f.workers.Done() + <-start + _, err := f.manager.ProcessScheduledPurchases(f.ctx) + worker <- err + }() close(start) select { case err := <-paused: @@ -295,6 +309,6 @@ func TestPauseClaim_ConcurrentPauseAndWorker(t *testing.T) { require.NoError(t, f.resume()) f.markNotified(t) f.service.unblock() - require.NoError(t, f.manager.ProcessMessage(f.ctx, f.body)) + f.sweep(t) f.assertCompleted(t) } diff --git a/internal/purchase/plan_step_retention_integration_test.go b/internal/purchase/plan_step_retention_integration_test.go index 30b531495..45a42c995 100644 --- a/internal/purchase/plan_step_retention_integration_test.go +++ b/internal/purchase/plan_step_retention_integration_test.go @@ -196,7 +196,11 @@ func TestCumulativeAttach_AuthorizedLegacyQuantityRemainsImmutable(t *testing.T) require.Equal(t, 1, result.Fired) require.Zero(t, result.Errored) } else { - require.NoError(t, f.execute(ctx, root)) + // An approved row is only ever executed by the stranded-approval + // re-drive funnel (RecoverStrandedApprovals). + claimed, redriveErr := f.manager.claimAndRedrive(ctx, rowStatusCopy(root)) + require.NoError(t, redriveErr) + require.True(t, claimed) } f.mu.Lock() counts := append([]int(nil), f.counts...) diff --git a/internal/purchase/ramp_step_progress_integration_test.go b/internal/purchase/ramp_step_progress_integration_test.go index 5f38bb41a..6a9753261 100644 --- a/internal/purchase/ramp_step_progress_integration_test.go +++ b/internal/purchase/ramp_step_progress_integration_test.go @@ -262,7 +262,7 @@ func (f *rampStepFixture) failedAccountExecution(ctx context.Context, t *testing // saveRetryExecution persists the successor row an operator's Retry produces // for one failed per-account row, mirroring api.persistRetryExecution: the // PlanID, StepNumber, CloudAccountID and idempotency lineage all propagate from -// the predecessor, the row arrives already approved by the human who clicked +// the predecessor, the row arrives pending, awaiting approval by the human who clicked // Retry, and the predecessor is stamped with retry_execution_id pointing at the // successor. That last write is what marks the failed row as superseded, so // omitting it would model a retry no production path can produce. @@ -275,7 +275,7 @@ func (f *rampStepFixture) saveRetryExecution(ctx context.Context, t *testing.T, IdempotencyKey: predecessor.IdempotencyKey, PlanID: f.plan.ID, CloudAccountID: &acctID, - Status: "approved", + Status: "pending", StepNumber: predecessor.StepNumber, ScheduledDate: time.Now(), Recommendations: []config.RecommendationRecord{withAccount(rampStepRecommendation()[0], accountID)}, @@ -300,10 +300,25 @@ func (f *rampStepFixture) saveRetryExecution(ctx context.Context, t *testing.T, return exec } -// execute drives the real async executor entry point for one row. -func (f *rampStepFixture) execute(ctx context.Context, exec *config.PurchaseExecution) error { - msg := AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: exec.ExecutionID} - return f.manager.handleExecutePurchase(ctx, &msg) +// executeRoot runs the due root row through the scheduler sweep, the production +// path for a notified plan row. A partial multi-account run counts as executed +// (the sweep's ack, #1014), so a clean tick is one executed row and no failures. +func (f *rampStepFixture) executeRoot(ctx context.Context) error { + result, err := f.manager.ProcessScheduledPurchases(ctx) + if err != nil { + return err + } + if result.Executed != 1 || result.Failed != 0 { + return fmt.Errorf("sweep executed %d and failed %d rows (errors: %v), want 1 and 0", result.Executed, result.Failed, result.Errors) + } + return nil +} + +// executeRetry approves the pending successor an operator's Retry persisted, +// the production path for a per-account retry row. +func (f *rampStepFixture) executeRetry(ctx context.Context, exec *config.PurchaseExecution) error { + _, _, err := f.manager.ApproveAndExecute(ctx, exec.ExecutionID, "", nil, nil) + return err } // TestRampStepAdvancesOncePerStepAcrossPerAccountRetries is the issue #1669 @@ -322,8 +337,8 @@ func TestRampStepAdvancesOncePerStepAcrossPerAccountRetries(t *testing.T) { require.Equal(t, 2, f.currentStep(ctx, t), "fixture must start on step 2 of 4") // Step 3 fans out: A commits, B and C fail credential resolution. - root := f.saveRootExecution(ctx, t, 3) - require.NoError(t, f.execute(ctx, root), + f.saveRootExecution(ctx, t, 3) + require.NoError(t, f.executeRoot(ctx), "a partial multi-account run must be acked, not surfaced as a flat failure (#1014)") require.Equal(t, 1, f.purchaseCount(), "exactly one account (A) may commit on the first pass") @@ -334,13 +349,13 @@ func TestRampStepAdvancesOncePerStepAcrossPerAccountRetries(t *testing.T) { f.repairAccounts(ctx, t) retryB := f.saveRetryExecution(ctx, t, f.accounts[1].ID) - require.NoError(t, f.execute(ctx, retryB)) + require.NoError(t, f.executeRetry(ctx, retryB)) require.Equal(t, 2, f.purchaseCount(), "B's retry must commit") assert.Equal(t, 2, f.currentStep(ctx, t), "B bought but C has not, so step 3 is not complete (issue #1861)") retryC := f.saveRetryExecution(ctx, t, f.accounts[2].ID) - require.NoError(t, f.execute(ctx, retryC)) + require.NoError(t, f.executeRetry(ctx, retryC)) require.Equal(t, 3, f.purchaseCount(), "C's retry must commit") assert.Equal(t, 3, f.currentStep(ctx, t), @@ -368,8 +383,8 @@ func TestRampStepAdvancesOncePerStepAcrossPerAccountRetries(t *testing.T) { // And the 4th step still counts. Pre-fix the ramp was already sitting at 4 // here, so this step's completion changed nothing and the plan finished // having bought three steps while reporting four. - step4 := f.saveRootExecution(ctx, t, 4) - require.NoError(t, f.execute(ctx, step4)) + f.saveRootExecution(ctx, t, 4) + require.NoError(t, f.executeRoot(ctx)) assert.Equal(t, 4, f.currentStep(ctx, t), "step 4 completes the ramp") assert.Equal(t, 6, f.purchaseCount(), "step 4 commits once per account") } @@ -392,15 +407,15 @@ func TestRampStepWaitsForEveryAccountBeforeAdvancing(t *testing.T) { require.Equal(t, 2, f.currentStep(ctx, t), "fixture must start on step 2 of 4") - root := f.saveRootExecution(ctx, t, 3) - require.NoError(t, f.execute(ctx, root)) + f.saveRootExecution(ctx, t, 3) + require.NoError(t, f.executeRoot(ctx)) require.Equal(t, 1, f.purchaseCount(), "exactly one account (A) may commit on the first pass") require.Equal(t, 2, f.currentStep(ctx, t), "a partially-failed step must not advance the ramp") // The operator repairs B only, and retries B only. C stays broken. f.repairAccount(ctx, t, 1) retryB := f.saveRetryExecution(ctx, t, f.accounts[1].ID) - require.NoError(t, f.execute(ctx, retryB)) + require.NoError(t, f.executeRetry(ctx, retryB)) require.Equal(t, 2, f.purchaseCount(), "B's retry must commit") assert.Equal(t, 2, f.currentStep(ctx, t), @@ -419,7 +434,7 @@ func TestRampStepWaitsForEveryAccountBeforeAdvancing(t *testing.T) { // exactly now the ramp advances. f.repairAccount(ctx, t, 2) retryC := f.saveRetryExecution(ctx, t, f.accounts[2].ID) - require.NoError(t, f.execute(ctx, retryC)) + require.NoError(t, f.executeRetry(ctx, retryC)) require.Equal(t, 3, f.purchaseCount(), "C's retry must commit") assert.Equal(t, 3, f.currentStep(ctx, t), @@ -442,8 +457,8 @@ func TestStuckRampStepIsReportedForPlanHealth(t *testing.T) { require.NoError(t, err) assert.NotContains(t, stuck, f.plan.ID, "a plan with no executions yet is not stuck") - root := f.saveRootExecution(ctx, t, 3) - require.NoError(t, f.execute(ctx, root)) + f.saveRootExecution(ctx, t, 3) + require.NoError(t, f.executeRoot(ctx)) stuck, err = f.store.GetStuckRampSteps(ctx) require.NoError(t, err) @@ -459,9 +474,9 @@ func TestStuckRampStepIsReportedForPlanHealth(t *testing.T) { assert.Equal(t, config.RampStepBlock{StepNumber: 3, StuckExecutions: 1}, stuck[f.plan.ID], "only C is still stuck once B has a retry pending") - require.NoError(t, f.execute(ctx, retryB)) + require.NoError(t, f.executeRetry(ctx, retryB)) retryC := f.saveRetryExecution(ctx, t, f.accounts[2].ID) - require.NoError(t, f.execute(ctx, retryC)) + require.NoError(t, f.executeRetry(ctx, retryC)) stuck, err = f.store.GetStuckRampSteps(ctx) require.NoError(t, err) diff --git a/internal/purchase/single_claim_integration_test.go b/internal/purchase/single_claim_integration_test.go index e36f1a6d8..3c0b9d958 100644 --- a/internal/purchase/single_claim_integration_test.go +++ b/internal/purchase/single_claim_integration_test.go @@ -38,7 +38,7 @@ func rowStatus(ctx context.Context, t *testing.T, f *rampStepFixture, id string) return row } -// Both orderings of the approve/claimAndExecute race: the second executor loses +// Both orderings of the approve/claim-funnel race: the second executor loses // the claim and never reaches the provider. Before the fix the approve path ran // an "approved" row unclaimed, so the second ordering bought it twice. func TestSingleClaim_BothOrderingsBuyOnce(t *testing.T) { @@ -48,10 +48,10 @@ func TestSingleClaim_BothOrderingsBuyOnce(t *testing.T) { require.NoError(t, err) require.True(t, global.AutoPurchaseEnabled) - t.Run("claimAndExecute then approve", func(t *testing.T) { + t.Run("claim funnel then approve", func(t *testing.T) { row := claimRaceRow(ctx, t, f, "notified") before := f.purchaseCount() - claimed, execErr := f.manager.claimAndExecute(ctx, rowStatusCopy(row)) + claimed, execErr := f.manager.claimAndExecuteWithin(ctx, rowStatusCopy(row), 0) require.NoError(t, execErr) require.True(t, claimed) _, _, err := f.manager.ApproveAndExecute(ctx, row.ExecutionID, "approver@example.invalid", nil, nil) @@ -60,12 +60,12 @@ func TestSingleClaim_BothOrderingsBuyOnce(t *testing.T) { assert.Equal(t, "completed", rowStatus(ctx, t, f, row.ExecutionID).Status) }) - t.Run("approve then claimAndExecute", func(t *testing.T) { + t.Run("approve then claim funnel", func(t *testing.T) { row := claimRaceRow(ctx, t, f, "notified") before := f.purchaseCount() _, _, err := f.manager.ApproveAndExecute(ctx, row.ExecutionID, "approver@example.invalid", nil, nil) require.NoError(t, err) - claimed, execErr := f.manager.claimAndExecute(ctx, rowStatusCopy(row)) + claimed, execErr := f.manager.claimAndExecuteWithin(ctx, rowStatusCopy(row), 0) require.NoError(t, execErr) assert.False(t, claimed, "the approved-and-run row must not be claimable again") assert.Equal(t, 1, f.purchaseCount()-before) @@ -90,7 +90,9 @@ func TestSingleClaim_ConcurrentExecutorsBuyOnce(t *testing.T) { direct := func(row *config.PurchaseExecution) { _, _, _ = f.manager.DirectExecute(ctx, row.ExecutionID, "a@example.invalid", nil, nil) } - cron := func(row *config.PurchaseExecution) { _, _ = f.manager.claimAndExecute(ctx, rowStatusCopy(row)) } + cron := func(row *config.PurchaseExecution) { + _, _ = f.manager.claimAndExecuteWithin(ctx, rowStatusCopy(row), 0) + } redrive := func(row *config.PurchaseExecution) { _, _ = f.manager.claimAndRedrive(ctx, rowStatusCopy(row)) } fire := func(row *config.PurchaseExecution) { f.manager.fireOneDue(ctx, rowStatusCopy(row)) } cases["notified"] = []func(*config.PurchaseExecution){approve, direct, cron} @@ -187,7 +189,7 @@ func TestSingleClaim_ReaperInterleavingRecoversPurchasedRow(t *testing.T) { } // Deterministic replay of the #829 race: while the approve path is inside its -// provider call, another executor (cron/SQS) tries to claim the same row. On the +// provider call, another executor ((cron)) tries to claim the same row. On the // old code the row sat "approved" and the second executor bought it again. func TestSingleClaim_ExecutorArrivingMidApproveCannotBuyAgain(t *testing.T) { ctx := context.Background() @@ -201,7 +203,7 @@ func TestSingleClaim_ExecutorArrivingMidApproveCannotBuyAgain(t *testing.T) { } c := rowStatusCopy(row) c.Status = "approved" - midClaimed, _ = f.manager.claimAndExecute(ctx, c) + midClaimed, _ = f.manager.claimAndExecuteWithin(ctx, c, 0) } _, _, err := f.manager.ApproveAndExecute(ctx, row.ExecutionID, "a@example.invalid", nil, nil) require.NoError(t, err) From 782fce160b007887c758d0b1c01703e6e54b5c1f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 15:58:01 +0100 Subject: [PATCH 07/14] fix(server): reject aws:sqs Lambda events as unrecognized Nothing in this repo produces an SQS event, so the async purchase path behind it was dead. An aws:sqs event now falls through to the "unrecognized Lambda event shape" error instead of running purchase execution without the 4-eyes and contact gates the HTTP path enforces. The new test and the converted detect, routing and cmd/lambda rows failed before this change (the event routed and returned nil). Refs #399 Co-Authored-By: Claude Opus 5.5 --- cmd/lambda/main_test.go | 8 ++++---- internal/server/lambda.go | 16 +--------------- internal/server/lambda_test.go | 31 +++++++++++++++++++++---------- 3 files changed, 26 insertions(+), 29 deletions(-) diff --git a/cmd/lambda/main_test.go b/cmd/lambda/main_test.go index 0f050897a..3a841414f 100644 --- a/cmd/lambda/main_test.go +++ b/cmd/lambda/main_test.go @@ -124,16 +124,16 @@ func TestHandler_ScheduledEvent(t *testing.T) { assert.NotNil(t, result) } -func TestHandler_SQSEvent(t *testing.T) { +func TestHandler_SQSEventIsRejected(t *testing.T) { origApp := app defer func() { app = origApp }() app = createTestApp() rawEvent := json.RawMessage(`{"Records":[{"eventSource":"aws:sqs","messageId":"msg-1","body":"{}"}]}`) - result, err := Handler(context.Background(), rawEvent) - require.NoError(t, err) - assert.NotNil(t, result) + _, err := Handler(context.Background(), rawEvent) + require.Error(t, err) + assert.Contains(t, err.Error(), "unrecognized") } func TestHandler_HTTPEvent(t *testing.T) { diff --git a/internal/server/lambda.go b/internal/server/lambda.go index 6d8e7ebd2..df2a7f6ec 100644 --- a/internal/server/lambda.go +++ b/internal/server/lambda.go @@ -38,15 +38,13 @@ func (app *Application) HandleLambdaEvent(ctx context.Context, rawEvent json.Raw switch eventType { case "http": return app.handleLambdaHTTPEvent(ctx, rawEvent) - case "sqs": - return app.handleLambdaSQSEvent(ctx, rawEvent) case "scheduled": return app.handleLambdaScheduledEvent(ctx, rawEvent) default: // Return a distinct error instead of silently treating an unrecognized // payload as a scheduled event. Masking the event shape as "unknown // scheduled task action" makes the real cause hard to diagnose (04-N4). - return nil, fmt.Errorf("unrecognized Lambda event shape (size %d bytes); not an HTTP/SQS/scheduled event", len(rawEvent)) + return nil, fmt.Errorf("unrecognized Lambda event shape (size %d bytes); not an HTTP/scheduled event", len(rawEvent)) } } @@ -67,18 +65,6 @@ func detectLambdaEventType(rawEvent json.RawMessage) string { } } - // Check for SQS event - var sqsEvent struct { - Records []struct { - EventSource string `json:"eventSource"` - } `json:"Records"` - } - if err := json.Unmarshal(rawEvent, &sqsEvent); err == nil { - if len(sqsEvent.Records) > 0 && sqsEvent.Records[0].EventSource == "aws:sqs" { - return "sqs" - } - } - // Check for EventBridge scheduled event var scheduledEvent struct { Source string `json:"source"` diff --git a/internal/server/lambda_test.go b/internal/server/lambda_test.go index da2824fd8..8059d1e2f 100644 --- a/internal/server/lambda_test.go +++ b/internal/server/lambda_test.go @@ -49,7 +49,7 @@ func TestDetectLambdaEventType(t *testing.T) { } ] }`, - expectedType: "sqs", + expectedType: "unknown", }, { name: "EventBridge scheduled event", @@ -528,6 +528,24 @@ func TestHandleLambdaEvent_UnknownEventReturnsError(t *testing.T) { "expected 'unrecognised' in error, got: "+err.Error()) } +// TestHandleLambdaEvent_SQSEventIsUnrecognized pins that an aws:sqs event no +// longer reaches purchase execution: nothing in this repo produces one, so a +// stray event-source mapping must fail loudly instead of running the purchase +// path without the gates the HTTP path enforces (issue #399). +func TestHandleLambdaEvent_SQSEventIsUnrecognized(t *testing.T) { + ctx := testutil.TestContext(t) + + app := &Application{ + API: api.NewHandler(api.HandlerConfig{}), + Purchase: &testutil.MockPurchaseManager{}, + } + + _, err := app.HandleLambdaEvent(ctx, json.RawMessage(`{"Records":[{"eventSource":"aws:sqs","messageId":"m","body":"{}"}]}`)) + testutil.AssertError(t, err) + testutil.AssertTrue(t, strings.Contains(err.Error(), "unrecognized Lambda event shape"), + "expected 'unrecognized Lambda event shape' in error, got: "+err.Error()) +} + func TestHandleLambdaEvent(t *testing.T) { tests := []struct { setupApp func(*Application) @@ -548,21 +566,14 @@ func TestHandleLambdaEvent(t *testing.T) { expectError: false, }, { - name: "SQS event routing", + name: "SQS event is rejected", rawEvent: `{ "Records": [{ "eventSource": "aws:sqs", "body": "{}" }] }`, - setupApp: func(app *Application) { - app.Purchase = &testutil.MockPurchaseManager{ - ProcessMessageFunc: func(ctx context.Context, body string) error { - return nil - }, - } - }, - expectError: false, + expectError: true, }, { name: "Scheduled event routing", From d6124e3b648f20837ce3ac985fff9170709b2186 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 17:56:48 +0100 Subject: [PATCH 08/14] chore(server): remove the dead SQS batch event handler With aws:sqs events no longer routed, handleLambdaSQSEvent and its batch-failure helper are unreachable. Delete them with their tests; the flag loader they used goes in the next commit. Refs #399 Co-Authored-By: Claude Opus 5.5 --- internal/server/lambda.go | 78 ----------- internal/server/lambda_test.go | 232 --------------------------------- 2 files changed, 310 deletions(-) diff --git a/internal/server/lambda.go b/internal/server/lambda.go index df2a7f6ec..aeda1ec5e 100644 --- a/internal/server/lambda.go +++ b/internal/server/lambda.go @@ -215,84 +215,6 @@ func isTextContentType(ct string) bool { return false } -// handleLambdaSQSEvent processes SQS messages (for async purchase processing). -// -// When app.appConfig.ReportSQSBatchItemFailures is true, returns a partial -// batch response (events.SQSEventResponse) naming only the records that -// actually failed, with a nil error, instead of a single error for the -// whole batch (issue #108). Lambda's SQS integration treats ANY non-nil -// handler error as "the entire batch failed" and redelivers every record, -// including ones HandleSQSMessage already processed successfully. On -// redelivery those already-succeeded records typically fail a second time -// (status already advanced past what the CAS-guarded handlers expect), -// which redelivers the whole batch again and burns the redrive budget on -// records that were never actually broken, until the one genuinely poison -// message drags the rest of the batch to the DLQ with it. -// -// The partial-response shape is only honored by AWS when the SQS -// event-source mapping sets function_response_types = -// ["ReportBatchItemFailures"] (Terraform; see loadReportSQSBatchItemFailures -// for why no such resource currently exists in this repo). Without that -// setting live, AWS ignores the response body entirely and treats a nil -// error as the WHOLE BATCH having succeeded -- deleting every message from -// the queue, including the ones that just failed. That is a strictly worse -// outcome than the old all-or-nothing behavior (which at least redelivered -// and eventually DLQ'd a genuine failure): a transient error would now -// silently drop an approval, and a poison message would vanish instead of -// landing in the DLQ for investigation. ReportSQSBatchItemFailures defaults -// to false specifically to keep the old, safe, all-or-nothing behavior -// until an operator has confirmed the event source mapping is actually -// configured to honor partial responses and flips -// SQS_REPORT_BATCH_ITEM_FAILURES=true. -// -// Regardless of the flag, a batch where every record failed always returns -// an error: there is no reason to ack (or partially ack) a batch that -// accomplished nothing, flag or no flag. -func (app *Application) handleLambdaSQSEvent(ctx context.Context, rawEvent json.RawMessage) (any, error) { - var sqsEvent events.SQSEvent - if err := json.Unmarshal(rawEvent, &sqsEvent); err != nil { - log.Printf("Failed to parse SQS event: %v", err) - return nil, err - } - - failures := []events.SQSBatchItemFailure{} - for _rvc := range sqsEvent.Records { - record := sqsEvent.Records[_rvc] - log.Printf("Processing SQS message: %s", record.MessageId) - if err := app.HandleSQSMessage(ctx, record.Body); err != nil { - log.Printf("Failed to process message %s: %v", record.MessageId, err) - failures = append(failures, events.SQSBatchItemFailure{ItemIdentifier: record.MessageId}) - } - } - - if len(failures) == 0 { - return events.SQSEventResponse{BatchItemFailures: failures}, nil - } - - if len(failures) == len(sqsEvent.Records) { - return nil, fmt.Errorf("failed to process all %d SQS message(s) in batch: %v", len(failures), sqsBatchItemFailureIDs(failures)) - } - - if !app.appConfig.ReportSQSBatchItemFailures { - // Fail closed: see the function doc comment above for why acking - // (even partially) without the Terraform side confirmed live would - // silently drop failed messages instead of redelivering them. - return nil, fmt.Errorf("failed to process %d SQS message(s): %v", len(failures), sqsBatchItemFailureIDs(failures)) - } - - return events.SQSEventResponse{BatchItemFailures: failures}, nil -} - -// sqsBatchItemFailureIDs extracts the message IDs from a batch item failure -// list for the aggregate-error log/error text. -func sqsBatchItemFailureIDs(failures []events.SQSBatchItemFailure) []string { - ids := make([]string, len(failures)) - for i, f := range failures { - ids[i] = f.ItemIdentifier - } - return ids -} - // loadReportSQSBatchItemFailures parses SQS_REPORT_BATCH_ITEM_FAILURES at // startup (called once from NewApplication). Unset means false: keep the // old all-or-nothing batch behavior, which is safe (if noisier) until an diff --git a/internal/server/lambda_test.go b/internal/server/lambda_test.go index 8059d1e2f..d42bae7f6 100644 --- a/internal/server/lambda_test.go +++ b/internal/server/lambda_test.go @@ -3,7 +3,6 @@ package server import ( "context" "encoding/json" - "fmt" "os" "strings" "testing" @@ -11,7 +10,6 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/api" "github.com/LeanerCloud/cloud-commitments-platform/internal/scheduler" "github.com/LeanerCloud/cloud-commitments-platform/internal/testutil" - "github.com/aws/aws-lambda-go/events" ) func TestDetectLambdaEventType(t *testing.T) { @@ -135,209 +133,6 @@ func TestHandleLambdaHTTPEvent(t *testing.T) { } } -func TestHandleLambdaSQSEvent(t *testing.T) { - tests := []struct { - setupMocks func(*testutil.MockPurchaseManager) - name string - rawEvent string - expectError bool - }{ - { - name: "valid SQS event with single message", - rawEvent: `{ - "Records": [ - { - "messageId": "msg-123", - "eventSource": "aws:sqs", - "body": "{\"purchase_id\": \"123\"}" - } - ] - }`, - setupMocks: func(p *testutil.MockPurchaseManager) { - p.ProcessMessageFunc = func(ctx context.Context, body string) error { - return nil - } - }, - expectError: false, - }, - { - name: "valid SQS event with multiple messages", - rawEvent: `{ - "Records": [ - { - "messageId": "msg-123", - "eventSource": "aws:sqs", - "body": "{\"purchase_id\": \"123\"}" - }, - { - "messageId": "msg-456", - "eventSource": "aws:sqs", - "body": "{\"purchase_id\": \"456\"}" - } - ] - }`, - setupMocks: func(p *testutil.MockPurchaseManager) { - callCount := 0 - p.ProcessMessageFunc = func(ctx context.Context, body string) error { - callCount++ - return nil - } - }, - expectError: false, - }, - { - name: "invalid JSON", - rawEvent: `{invalid json}`, - setupMocks: func(p *testutil.MockPurchaseManager) {}, - expectError: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - ctx := testutil.TestContext(t) - - mockPurchase := &testutil.MockPurchaseManager{} - tt.setupMocks(mockPurchase) - - app := &Application{ - Purchase: mockPurchase, - } - - _, err := app.handleLambdaSQSEvent(ctx, json.RawMessage(tt.rawEvent)) - - if tt.expectError { - testutil.AssertError(t, err) - } else { - testutil.AssertNoError(t, err) - } - }) - } -} - -// partialFailureRawEvent is a 3-record batch where only "msg-2" fails, -// shared by the flag-on and flag-off partial-failure tests below. -const partialFailureRawEvent = `{ - "Records": [ - {"messageId": "msg-1", "eventSource": "aws:sqs", "body": "{\"execution_id\": \"msg-1\"}"}, - {"messageId": "msg-2", "eventSource": "aws:sqs", "body": "{\"execution_id\": \"msg-2\"}"}, - {"messageId": "msg-3", "eventSource": "aws:sqs", "body": "{\"execution_id\": \"msg-3\"}"} - ] -}` - -func failOnMsg2Purchase() *testutil.MockPurchaseManager { - return &testutil.MockPurchaseManager{ - ProcessMessageFunc: func(_ context.Context, body string) error { - if strings.Contains(body, "msg-2") { - return fmt.Errorf("execution not found: msg-2") - } - return nil - }, - } -} - -// TestHandleLambdaSQSEvent_PartialBatchFailure_FlagOn pins issue #108's -// fix, gated behind ReportSQSBatchItemFailures=true: a batch where only -// some records fail must leave the succeeded records ACKed rather than -// redelivering the whole batch. Pre-fix, handleLambdaSQSEvent collected -// every failed message ID and returned a single aggregate error for the -// batch; Lambda's SQS integration treats ANY non-nil handler error as "the -// entire batch failed", so records 1 and 3 (which HandleSQSMessage already -// processed successfully) would be redelivered right alongside the -// genuinely failed record 2 -- and would typically fail a second time -// since their status already advanced past what the CAS-guarded handlers -// expect, repeating until the whole batch (not just the poison message) -// hits the DLQ. -// -// Asserted on the returned events.SQSEventResponse.BatchItemFailures set, -// per the issue's own fix direction, rather than on the error value: the -// function must return a nil error alongside the partial-failure response, -// since a non-nil error here is exactly the all-or-nothing behavior being -// removed. -func TestHandleLambdaSQSEvent_PartialBatchFailure_FlagOn(t *testing.T) { - ctx := testutil.TestContext(t) - - app := &Application{ - Purchase: failOnMsg2Purchase(), - appConfig: ApplicationConfig{ReportSQSBatchItemFailures: true}, - } - - result, err := app.handleLambdaSQSEvent(ctx, json.RawMessage(partialFailureRawEvent)) - testutil.AssertNoError(t, err) - - resp, ok := result.(events.SQSEventResponse) - if !ok { - t.Fatalf("expected events.SQSEventResponse, got %T", result) - } - if len(resp.BatchItemFailures) != 1 { - t.Fatalf("expected exactly 1 batch item failure, got %d: %+v", len(resp.BatchItemFailures), resp.BatchItemFailures) - } - if resp.BatchItemFailures[0].ItemIdentifier != "msg-2" { - t.Fatalf("expected the failed record's own messageId (msg-2) in BatchItemFailures, got %q", - resp.BatchItemFailures[0].ItemIdentifier) - } -} - -// TestHandleLambdaSQSEvent_PartialBatchFailure_FlagOff pins the review -// finding on PR #382: without the SQS event source mapping's -// function_response_types actually including "ReportBatchItemFailures", -// AWS ignores the events.SQSEventResponse body entirely, so returning a -// nil error there means the WHOLE BATCH is acked and deleted -- silently -// dropping the genuinely failed record instead of redelivering it. With -// ReportSQSBatchItemFailures left at its default (false), a partial -// failure must still return the old aggregate error so the whole batch is -// redelivered (safe, if noisier) until an operator confirms the Terraform -// side is live and flips the flag. -func TestHandleLambdaSQSEvent_PartialBatchFailure_FlagOff(t *testing.T) { - ctx := testutil.TestContext(t) - - // Zero-value Application.appConfig: ReportSQSBatchItemFailures defaults - // to false, matching a real, unconfigured deployment. - app := &Application{Purchase: failOnMsg2Purchase()} - - result, err := app.handleLambdaSQSEvent(ctx, json.RawMessage(partialFailureRawEvent)) - testutil.AssertError(t, err) - if result != nil { - t.Fatalf("expected a nil result alongside the aggregate error, got %+v", result) - } - if !strings.Contains(err.Error(), "msg-2") { - t.Fatalf("expected the aggregate error to name the failed message, got: %v", err) - } -} - -// TestHandleLambdaSQSEvent_AllFailed_ReturnsErrorRegardlessOfFlag pins the -// review finding that a batch where every record failed must never be -// acked, flag or no flag -- there is nothing to partially succeed on, so -// returning the partial-success shape (or a nil error) would ack a batch -// that accomplished nothing. -func TestHandleLambdaSQSEvent_AllFailed_ReturnsErrorRegardlessOfFlag(t *testing.T) { - for _, flagValue := range []bool{true, false} { - t.Run(fmt.Sprintf("flag=%v", flagValue), func(t *testing.T) { - ctx := testutil.TestContext(t) - - app := &Application{ - Purchase: &testutil.MockPurchaseManager{ - ProcessMessageFunc: func(_ context.Context, _ string) error { - return fmt.Errorf("execution not found") - }, - }, - appConfig: ApplicationConfig{ReportSQSBatchItemFailures: flagValue}, - } - - rawEvent := `{"Records": [ - {"messageId": "msg-1", "eventSource": "aws:sqs", "body": "{}"}, - {"messageId": "msg-2", "eventSource": "aws:sqs", "body": "{}"} - ]}` - - result, err := app.handleLambdaSQSEvent(ctx, json.RawMessage(rawEvent)) - testutil.AssertError(t, err) - if result != nil { - t.Fatalf("expected a nil result alongside the all-failed error, got %+v", result) - } - }) - } -} - // TestLoadReportSQSBatchItemFailures pins the "error on garbage" contract // for SQS_REPORT_BATCH_ITEM_FAILURES: unset defaults to false (fail // closed), a recognized boolean parses, and anything else is a startup @@ -380,33 +175,6 @@ func TestLoadReportSQSBatchItemFailures(t *testing.T) { } } -// TestHandleLambdaSQSEvent_AllSucceedReturnsEmptyFailures documents the -// all-succeed shape: a nil error and an explicitly empty (not omitted) -// BatchItemFailures list, matching the SQS partial-batch-response contract. -// This holds regardless of ReportSQSBatchItemFailures, since an empty -// failure list is equivalent to plain success either way. -func TestHandleLambdaSQSEvent_AllSucceedReturnsEmptyFailures(t *testing.T) { - ctx := testutil.TestContext(t) - - mockPurchase := &testutil.MockPurchaseManager{ - ProcessMessageFunc: func(_ context.Context, _ string) error { return nil }, - } - app := &Application{Purchase: mockPurchase} - - rawEvent := `{"Records": [{"messageId": "msg-1", "eventSource": "aws:sqs", "body": "{}"}]}` - - result, err := app.handleLambdaSQSEvent(ctx, json.RawMessage(rawEvent)) - testutil.AssertNoError(t, err) - - resp, ok := result.(events.SQSEventResponse) - if !ok { - t.Fatalf("expected events.SQSEventResponse, got %T", result) - } - if len(resp.BatchItemFailures) != 0 { - t.Fatalf("expected no batch item failures, got %+v", resp.BatchItemFailures) - } -} - // testOwnerToken is a fixed, well-formed collection owner token. It has to be // a real UUID because ParseScheduledEvent rejects malformed non-empty tokens // at the boundary rather than letting them reach the UUID-typed column. From 55188a6b75b4d8e33f17b93a1a157f074003f2cc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 17:57:32 +0100 Subject: [PATCH 09/14] chore(server): remove SQS_REPORT_BATCH_ITEM_FAILURES and HandleSQSMessage Delete the flag, its loader and startup validation, Application .HandleSQSMessage, PurchaseManagerInterface.ProcessMessage, the mock's ProcessMessageFunc and their tests. The os and strconv imports in lambda.go go with the loader. Refs #399 Co-Authored-By: Claude Opus 5.5 --- internal/server/app.go | 13 --------- internal/server/app_test.go | 2 +- internal/server/handler.go | 11 ------- internal/server/handler_test.go | 51 --------------------------------- internal/server/interfaces.go | 1 - internal/server/lambda.go | 26 ----------------- internal/server/lambda_test.go | 43 --------------------------- internal/testutil/mocks.go | 8 ------ 8 files changed, 1 insertion(+), 154 deletions(-) diff --git a/internal/server/app.go b/internal/server/app.go index 2095756fd..c50f913ec 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -151,13 +151,6 @@ type ApplicationConfig struct { // X-Forwarded-For before the app (CUDLY_TRUSTED_PROXY_HOPS). 0 ignores // the header and uses the peer address. TrustedProxyHops int - // ReportSQSBatchItemFailures gates whether handleLambdaSQSEvent returns - // the partial events.SQSEventResponse{BatchItemFailures} shape (true) or - // the old whole-batch aggregate error (false, default). See - // loadReportSQSBatchItemFailures for why this must stay false until the - // SQS event source mapping's function_response_types actually includes - // "ReportBatchItemFailures" (issue #108 follow-up). - ReportSQSBatchItemFailures bool // RecommendationSchedule is RECOMMENDATION_SCHEDULE: the schedule // Terraform gave the deployment's recommendation-collection trigger // (EventBridge rate()/cron() or five-field cron), empty when scheduled @@ -666,12 +659,6 @@ func NewApplication(ctx context.Context, version string) (*Application, error) { return nil, fmt.Errorf("invalid analytics configuration: %w", err) } - reportSQSBatchItemFailures, err := loadReportSQSBatchItemFailures() - if err != nil { - return nil, fmt.Errorf("invalid SQS configuration: %w", err) - } - cfg.ReportSQSBatchItemFailures = reportSQSBatchItemFailures - log.Printf("CUDly Server initializing, version: %s", cfg.Version) // Initialize configuration store (PostgreSQL). The store connects lazily on diff --git a/internal/server/app_test.go b/internal/server/app_test.go index 92140fdff..2713ef82a 100644 --- a/internal/server/app_test.go +++ b/internal/server/app_test.go @@ -431,7 +431,7 @@ func TestLoadApplicationConfig(t *testing.T) { } func TestNewApplicationRejectsMalformedEnvDefaults(t *testing.T) { - for _, key := range []string{"DEFAULT_TERM", "DEFAULT_COVERAGE", "SCHEDULED_PURCHASE_MAX_OVERDUE", "ANALYTICS_COLLECTION_ENABLED", "ANALYTICS_RETENTION_MONTHS", "ANALYTICS_PARTITIONS_AHEAD", "SQS_REPORT_BATCH_ITEM_FAILURES", "DB_HOST"} { + for _, key := range []string{"DEFAULT_TERM", "DEFAULT_COVERAGE", "SCHEDULED_PURCHASE_MAX_OVERDUE", "ANALYTICS_COLLECTION_ENABLED", "ANALYTICS_RETENTION_MONTHS", "ANALYTICS_PARTITIONS_AHEAD", "DB_HOST"} { t.Setenv(key, "") } for _, tc := range []struct{ key, value string }{ diff --git a/internal/server/handler.go b/internal/server/handler.go index 1606e68b6..4ee176a16 100644 --- a/internal/server/handler.go +++ b/internal/server/handler.go @@ -418,17 +418,6 @@ func (app *Application) handleRefreshAnalytics(ctx context.Context) (map[string] return result, nil } -// HandleSQSMessage processes an SQS message for async purchase processing. -func (app *Application) HandleSQSMessage(ctx context.Context, body string) error { - log.Printf("Processing SQS message (size: %d bytes)", len(body)) - if err := app.Purchase.ProcessMessage(ctx, body); err != nil { - log.Printf("Failed to process SQS message: %v", err) - return err - } - log.Println("SQS message processed successfully") - return nil -} - // ScheduledEvent represents a generic scheduled event. type ScheduledEvent struct { Source string `json:"source"` diff --git a/internal/server/handler_test.go b/internal/server/handler_test.go index 60ee129bd..c3e5a964f 100644 --- a/internal/server/handler_test.go +++ b/internal/server/handler_test.go @@ -417,57 +417,6 @@ func TestHandleScheduledTaskReleasesMarkerWhenSkipped(t *testing.T) { }) } -func TestHandleSQSMessage(t *testing.T) { - tests := []struct { - setupMocks func(*testutil.MockPurchaseManager) - name string - messageBody string - expectError bool - }{ - { - name: "valid message", - messageBody: `{"purchase_id": "123"}`, - setupMocks: func(p *testutil.MockPurchaseManager) { - p.ProcessMessageFunc = func(ctx context.Context, body string) error { - return nil - } - }, - expectError: false, - }, - { - name: "invalid message", - messageBody: `{"invalid": "data"}`, - setupMocks: func(p *testutil.MockPurchaseManager) { - p.ProcessMessageFunc = func(ctx context.Context, body string) error { - return errors.New("invalid message format") - } - }, - expectError: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - ctx := testutil.TestContext(t) - - mockPurchase := &testutil.MockPurchaseManager{} - tt.setupMocks(mockPurchase) - - app := &Application{ - Purchase: mockPurchase, - } - - err := app.HandleSQSMessage(ctx, tt.messageBody) - - if tt.expectError { - testutil.AssertError(t, err) - } else { - testutil.AssertNoError(t, err) - } - }) - } -} - // TestParseScheduledEvent_MalformedTokenNotEchoedInError pins that the // rejection error for a malformed owner_token never carries any of the // rejected value. The 45-character input below is the one shape where diff --git a/internal/server/interfaces.go b/internal/server/interfaces.go index e3d9d9ec6..94f02094a 100644 --- a/internal/server/interfaces.go +++ b/internal/server/interfaces.go @@ -30,7 +30,6 @@ type SchedulerInterface interface { type PurchaseManagerInterface interface { ProcessScheduledPurchases(ctx context.Context) (*purchase.ProcessResult, error) SendUpcomingPurchaseNotifications(ctx context.Context) (*purchase.NotificationResult, error) - ProcessMessage(ctx context.Context, body string) error // ApproveExecution, ApproveAndExecute and RunPlannedPurchaseNow return the // raw revocation token minted on a successful execute (issue #103: only // its hash is stored, so a re-read cannot yield an emailable token). diff --git a/internal/server/lambda.go b/internal/server/lambda.go index aeda1ec5e..35a389211 100644 --- a/internal/server/lambda.go +++ b/internal/server/lambda.go @@ -6,8 +6,6 @@ import ( "encoding/json" "fmt" "log" - "os" - "strconv" "strings" "github.com/LeanerCloud/cloud-commitments-platform/internal/api" @@ -215,30 +213,6 @@ func isTextContentType(ct string) bool { return false } -// loadReportSQSBatchItemFailures parses SQS_REPORT_BATCH_ITEM_FAILURES at -// startup (called once from NewApplication). Unset means false: keep the -// old all-or-nothing batch behavior, which is safe (if noisier) until an -// operator confirms the SQS event source mapping's function_response_types -// actually includes "ReportBatchItemFailures" -- a Terraform resource that -// does not exist anywhere in this repo today (issue #108 follow-up: no -// aws_sqs_queue / AWS::SQS::Queue / EventSourceMapping / batch_size found -// across terraform/, cloudformation/, iac/, or arm/). A set but -// unparseable value is a startup error rather than a silently-ignored -// typo: flipping this flag is a deploy-time contract with that Terraform -// setting, and getting it wrong risks silently dropping failed purchase -// messages instead of redelivering them (see handleLambdaSQSEvent). -func loadReportSQSBatchItemFailures() (bool, error) { - raw := os.Getenv("SQS_REPORT_BATCH_ITEM_FAILURES") - if raw == "" { - return false, nil - } - val, err := strconv.ParseBool(raw) - if err != nil { - return false, fmt.Errorf("invalid SQS_REPORT_BATCH_ITEM_FAILURES %q: must be a boolean: %w", raw, err) - } - return val, nil -} - // handleLambdaScheduledEvent processes scheduled/cron events. func (app *Application) handleLambdaScheduledEvent(ctx context.Context, rawEvent json.RawMessage) (any, error) { taskType, params, err := ParseScheduledEvent(rawEvent) diff --git a/internal/server/lambda_test.go b/internal/server/lambda_test.go index d42bae7f6..a09c28049 100644 --- a/internal/server/lambda_test.go +++ b/internal/server/lambda_test.go @@ -3,7 +3,6 @@ package server import ( "context" "encoding/json" - "os" "strings" "testing" @@ -133,48 +132,6 @@ func TestHandleLambdaHTTPEvent(t *testing.T) { } } -// TestLoadReportSQSBatchItemFailures pins the "error on garbage" contract -// for SQS_REPORT_BATCH_ITEM_FAILURES: unset defaults to false (fail -// closed), a recognized boolean parses, and anything else is a startup -// error rather than a silently-ignored typo. -func TestLoadReportSQSBatchItemFailures(t *testing.T) { - t.Cleanup(func() { os.Unsetenv("SQS_REPORT_BATCH_ITEM_FAILURES") }) - - tests := []struct { - name string - envValue string - unset bool - wantVal bool - wantError bool - }{ - {name: "unset defaults to false", unset: true, wantVal: false}, - {name: "true", envValue: "true", wantVal: true}, - {name: "false", envValue: "false", wantVal: false}, - {name: "garbage errors", envValue: "yes-please", wantError: true}, - {name: "empty string treated as unset", envValue: "", wantVal: false}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if tt.unset { - os.Unsetenv("SQS_REPORT_BATCH_ITEM_FAILURES") - } else { - os.Setenv("SQS_REPORT_BATCH_ITEM_FAILURES", tt.envValue) - } - - got, err := loadReportSQSBatchItemFailures() - if tt.wantError { - testutil.AssertError(t, err) - return - } - testutil.AssertNoError(t, err) - if got != tt.wantVal { - t.Fatalf("expected %v, got %v", tt.wantVal, got) - } - }) - } -} - // testOwnerToken is a fixed, well-formed collection owner token. It has to be // a real UUID because ParseScheduledEvent rejects malformed non-empty tokens // at the boundary rather than letting them reach the UUID-typed column. diff --git a/internal/testutil/mocks.go b/internal/testutil/mocks.go index 3cb45bd67..ddc1cd07d 100644 --- a/internal/testutil/mocks.go +++ b/internal/testutil/mocks.go @@ -60,7 +60,6 @@ func (m *MockScheduler) GetRecommendationByID(ctx context.Context, id string) (* type MockPurchaseManager struct { ProcessScheduledPurchasesFunc func(ctx context.Context) (*purchase.ProcessResult, error) SendUpcomingPurchaseNotificationsFunc func(ctx context.Context) (*purchase.NotificationResult, error) - ProcessMessageFunc func(ctx context.Context, body string) error ApproveExecutionFunc func(ctx context.Context, execID, token, actor string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) ApproveAndExecuteFunc func(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) DirectExecuteFunc func(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) @@ -85,13 +84,6 @@ func (m *MockPurchaseManager) SendUpcomingPurchaseNotifications(ctx context.Cont return &purchase.NotificationResult{}, nil } -func (m *MockPurchaseManager) ProcessMessage(ctx context.Context, body string) error { - if m.ProcessMessageFunc != nil { - return m.ProcessMessageFunc(ctx, body) - } - return nil -} - func (m *MockPurchaseManager) ApproveExecution(ctx context.Context, execID, token, actor string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) { if m.ApproveExecutionFunc != nil { return m.ApproveExecutionFunc(ctx, execID, token, actor, check) From d9bee588bcbcad190e93c172a047ad21cdc0e689 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 17:58:00 +0100 Subject: [PATCH 10/14] chore(purchase): remove the SQS approve and cancel message handlers handleApproveMessage, handleCancelMessage and their private approver gate (verifyAsyncApprovalActor, loadAsyncExecutionForApproval, matchActorAgainstApprovers, gatherApproverContactEmails) had no caller outside ProcessMessage. The production approver gate is api.Handler.authorizeApprovalAction. Trim the approve and cancel cases, message types and the token/actor fields from the message envelope, and drop the two missing-token subtests (twin: TestManager_ApproveExecution_EmptyToken and the CancelExecution invalid-token test). Refs #399 Co-Authored-By: Claude Opus 5.5 --- internal/purchase/messages.go | 188 ----------------------------- internal/purchase/messages_test.go | 26 ---- 2 files changed, 214 deletions(-) diff --git a/internal/purchase/messages.go b/internal/purchase/messages.go index 587137910..df3b6d7a9 100644 --- a/internal/purchase/messages.go +++ b/internal/purchase/messages.go @@ -5,11 +5,9 @@ import ( "encoding/json" "errors" "fmt" - "strings" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" - "github.com/LeanerCloud/cloud-commitments-platform/internal/email" ) // MessageType defines the types of async messages that can be processed. @@ -18,38 +16,20 @@ type MessageType string const ( // MessageTypeExecutePurchase triggers execution of a scheduled purchase. MessageTypeExecutePurchase MessageType = "execute_purchase" - // MessageTypeApprove approves a pending execution. - MessageTypeApprove MessageType = "approve" - // MessageTypeCancel cancels a pending execution. - MessageTypeCancel MessageType = "cancel" // MessageTypeSendNotification sends a notification for upcoming purchase. MessageTypeSendNotification MessageType = "send_notification" ) // AsyncMessage represents an SQS message for async purchase processing. -// -// ActorEmail is REQUIRED for approve/cancel messages: the SQS path must -// carry the verified session email of the operator who initiated the -// action so the consumer can enforce the same per-account contact_email -// gate as the HTTP path's authorizeApprovalAction. Messages without -// ActorEmail are rejected (no tokenless legacy fallback) — this is the -// CRITICAL hardening for replayed/forwarded payloads. Producers in this -// repo have always paired token+actor; legacy in-flight messages -// pre-dating this field are rejected by design and the operator can -// re-issue the action via the HTTP route. type AsyncMessage struct { Type MessageType `json:"type"` ExecutionID string `json:"execution_id,omitempty"` PlanID string `json:"plan_id,omitempty"` - Token string `json:"token,omitempty"` - ActorEmail string `json:"actor_email,omitempty"` } // ProcessMessage handles SQS messages for async purchase processing. // Supported message types: // - execute_purchase: Execute a scheduled purchase by execution_id -// - approve: Approve a pending execution (requires execution_id and token) -// - cancel: Cancel a pending execution (requires execution_id and token) // - send_notification: Send notification for upcoming purchases func (m *Manager) ProcessMessage(ctx context.Context, body string) error { logging.Debug("Processing async message") @@ -65,10 +45,6 @@ func (m *Manager) ProcessMessage(ctx context.Context, body string) error { switch msg.Type { case MessageTypeExecutePurchase: return m.handleExecutePurchase(ctx, &msg) - case MessageTypeApprove: - return m.handleApproveMessage(ctx, &msg) - case MessageTypeCancel: - return m.handleCancelMessage(ctx, &msg) case MessageTypeSendNotification: _, err := m.SendUpcomingPurchaseNotifications(ctx) return err @@ -153,167 +129,3 @@ func (m *Manager) handleExecutePurchase(ctx context.Context, msg *AsyncMessage) } return execErr } - -// handleApproveMessage processes an approve message. -// -// SECURITY: closes the previous async-SQS bypass of the approver gate. -// The handler now requires an actor_email field on the message and -// verifies it against the same per-account contact_email approver list -// that authorizeApprovalAction enforces on the HTTP path (see -// internal/api/handler_purchases.go). Token validation alone is no -// longer sufficient — a replayed or forwarded payload without a -// matching actor is rejected. -// -// Legacy in-flight messages produced before this field existed will be -// rejected (option (a) in the hardening plan): we preferred a clean -// reject over a tokenless backfill because reopening the tokenless path -// for any reason is exactly the threat model this fix addresses. Any -// legitimate stranded action can be re-issued via the HTTP route gated -// by authorizeApprovalAction. -func (m *Manager) handleApproveMessage(ctx context.Context, msg *AsyncMessage) error { - if msg.ExecutionID == "" || msg.Token == "" { - return fmt.Errorf("execution_id and token required for approve message") - } - if err := m.verifyAsyncApprovalActor(ctx, msg); err != nil { - return err - } - // The SQS worker sends no email itself, so the final row and the minted - // revocation token (issue #103) have no consumer here; only the error - // matters. It carries no session, so there are no permission Constraints - // to re-check against the claimed row (nil ClaimedRowCheck, issue #415). - _, _, err := m.ApproveExecution(ctx, msg.ExecutionID, msg.Token, msg.ActorEmail, nil) - return err -} - -// handleCancelMessage processes a cancel message. Same hardening as -// handleApproveMessage: token + actor_email + approver-list match are -// all required. -func (m *Manager) handleCancelMessage(ctx context.Context, msg *AsyncMessage) error { - if msg.ExecutionID == "" || msg.Token == "" { - return fmt.Errorf("execution_id and token required for cancel message") - } - if err := m.verifyAsyncApprovalActor(ctx, msg); err != nil { - return err - } - return m.CancelExecution(ctx, msg.ExecutionID, msg.Token, msg.ActorEmail) -} - -// verifyAsyncApprovalActor enforces the same per-account contact_email -// approver gate the HTTP path runs through authorizeApprovalAction -// (internal/api/handler_purchases.go) before approve/cancel mutates -// state. Steps: -// -// 1. Reject the message outright if actor_email is empty — legacy / -// replayed payloads land here and must NOT fall through to a -// tokenless approval. -// 2. Load the execution by ID and validate the approval token (constant- -// time compare plus TTL) through ValidateApprovalToken, the same -// helper ApproveExecution / CancelExecution use, so a bad or expired -// token fails fast with the same error shape and we don't leak -// whether the actor or the token was wrong. -// 3. Resolve the per-account contact_email approver list from the -// execution's recommendations and assert actor_email matches one -// of them (case-insensitive, trimmed). Empty approver list is a -// hard reject — same policy as authorizeApprovalAction. -// -// Returning an error here causes ProcessMessage to surface it to the -// SQS layer; the message is not re-driven into ApproveExecution / -// CancelExecution. -func (m *Manager) verifyAsyncApprovalActor(ctx context.Context, msg *AsyncMessage) error { - if strings.TrimSpace(msg.ActorEmail) == "" { - return fmt.Errorf("actor_email required for approve/cancel message") - } - execution, err := m.loadAsyncExecutionForApproval(ctx, msg) - if err != nil { - return err - } - return m.matchActorAgainstApprovers(ctx, msg.ActorEmail, execution.Recommendations) -} - -// loadAsyncExecutionForApproval fetches the execution targeted by an -// async approve/cancel message and validates the approval token, -// including its expiry, via ValidateApprovalToken. Split -// out from verifyAsyncApprovalActor so the parent stays under the -// repo's gocyclo threshold. -func (m *Manager) loadAsyncExecutionForApproval(ctx context.Context, msg *AsyncMessage) (*config.PurchaseExecution, error) { - execution, err := m.config.GetExecutionByID(ctx, msg.ExecutionID) - if errors.Is(err, config.ErrNotFound) { - return nil, fmt.Errorf("execution not found: %s", msg.ExecutionID) - } - if err != nil { - return nil, fmt.Errorf("failed to get execution: %w", err) - } - // Same check as ApproveExecution and loadCancelableExecution (issue - // #268): non-empty token, constant-time digest compare against the - // SHA-256 hash stored at rest (issue #103), and the TTL (issue #397). - if err := ValidateApprovalToken(execution, msg.Token); err != nil { - return nil, err - } - return execution, nil -} - -// matchActorAgainstApprovers resolves the per-account contact_email -// approver list for an execution and asserts the actor matches one of -// them (case-insensitive, trimmed). Empty approver list is a hard -// reject — mirrors authorizeApprovalAction's policy. -func (m *Manager) matchActorAgainstApprovers(ctx context.Context, actor string, recs []config.RecommendationRecord) error { - approvers, err := m.gatherApproverContactEmails(ctx, recs) - if err != nil { - return fmt.Errorf("failed to resolve approvers: %w", err) - } - if len(approvers) == 0 { - return fmt.Errorf("no per-account contact email configured for this execution; set the cloud account's contact_email before approving") - } - if email.ContainsAddress(approvers, actor) { - return nil - } - return fmt.Errorf("actor email is not an authorized approver for this purchase") -} - -// gatherApproverContactEmails mirrors the algorithm in -// internal/api/handler_purchases.go gatherAccountContactEmails: dedup -// by lowercase, preserve insertion order, skip recs without a -// CloudAccountID, skip accounts that are not found or have an empty -// ContactEmail. -// -// The two implementations are deliberately duplicated rather than -// extracted into a shared package: internal/api and internal/purchase -// share neither a parent package nor a transitive dependency, and -// hoisting one helper into config or a new package costs more than -// keeping the ~15-line loop in sync. If a third caller appears, hoist -// it then. Until then, each side cross-references the other so future -// edits don't drift. -func (m *Manager) gatherApproverContactEmails(ctx context.Context, recs []config.RecommendationRecord) ([]string, error) { - seenAccount := map[string]bool{} - seenEmail := map[string]bool{} - var out []string - for i := range recs { - rec := &recs[i] - if rec.CloudAccountID == nil || *rec.CloudAccountID == "" { - continue - } - id := *rec.CloudAccountID - if seenAccount[id] { - continue - } - seenAccount[id] = true - acct, err := m.config.GetCloudAccount(ctx, id) - if err != nil { - return nil, fmt.Errorf("failed to get cloud account %s: %w", id, err) - } - if acct == nil { - continue - } - addr := strings.TrimSpace(acct.ContactEmail) - if addr == "" { - continue - } - norm := strings.ToLower(addr) - if seenEmail[norm] { - continue - } - seenEmail[norm] = true - out = append(out, addr) - } - return out, nil -} diff --git a/internal/purchase/messages_test.go b/internal/purchase/messages_test.go index 149064f84..ac1ee4e62 100644 --- a/internal/purchase/messages_test.go +++ b/internal/purchase/messages_test.go @@ -53,32 +53,6 @@ func TestManager_ProcessMessage(t *testing.T) { assert.Contains(t, err.Error(), "execution_id required") }) - t.Run("approve missing token", func(t *testing.T) { - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - err := manager.ProcessMessage(ctx, `{"type": "approve", "execution_id": "exec-123"}`) - assert.Error(t, err) - assert.Contains(t, err.Error(), "token required") - }) - - t.Run("cancel missing token", func(t *testing.T) { - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - err := manager.ProcessMessage(ctx, `{"type": "cancel", "execution_id": "exec-123"}`) - assert.Error(t, err) - assert.Contains(t, err.Error(), "token required") - }) - t.Run("send_notification success", func(t *testing.T) { mockStore := new(MockConfigStore) mockEmail := new(MockEmailSender) From 2f0405585a40d015474a8feb440faf622197d40e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 17:58:41 +0100 Subject: [PATCH 11/14] chore(purchase): remove ProcessMessage and the claimAndExecute wrapper Delete the rest of messages.go (ProcessMessage, handleExecutePurchase, checkAutoExecuteGate, the message types) and its tests, plus the claimAndExecute wrapper whose last callers were re-homed. The cron sweep keeps claimAndExecuteWithin, isMultiAccountAckable and executableByScheduler. The by-ID execute loader and its "approved rows bypass the gate" rule had no production caller. Refs #399 Co-Authored-By: Claude Opus 5.5 --- internal/purchase/manager.go | 41 ++-- internal/purchase/messages.go | 131 ------------ internal/purchase/messages_test.go | 187 ------------------ .../redshift_gate_integration_test.go | 12 +- 4 files changed, 24 insertions(+), 347 deletions(-) delete mode 100644 internal/purchase/messages.go delete mode 100644 internal/purchase/messages_test.go diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index b719366bc..710f170d5 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -180,15 +180,14 @@ func (m *Manager) finalizeExecution(exec *config.PurchaseExecution, execErr erro } } -// claimAndExecute is the single atomic claim-then-execute funnel for the -// non-synchronous executor entry points (SQS execute_purchase and the cron -// ProcessScheduledPurchases sweep), mirroring the guard the synchronous approve -// path already gets from ApproveAndExecute's CAS (issue #1013). +// claimAndExecuteWithin is the single atomic claim-then-execute funnel for the +// cron ProcessScheduledPurchases sweep, mirroring the guard the synchronous +// approve path already gets from ApproveAndExecute's CAS (issue #1013). // // It atomically transitions the row from an executable state // (approved/pending/notified) to "running" via TransitionExecutionStatus and // only proceeds when it wins that CAS. A lost CAS (row already claimed by a -// concurrent worker, an overlapping cron tick, an SQS redelivery, or the row +// concurrent worker, an overlapping cron tick, or the row // vanishing mid-flight) is benign: the function returns (claimed=false, nil) and // the caller should ack/skip without re-running the purchase. A real DB error // during the claim returns (false, err). @@ -204,21 +203,17 @@ func (m *Manager) finalizeExecution(exec *config.PurchaseExecution, execErr erro // step cap is re-checked against the claimed row and the current plan; a refusal // fails the row with the beforeSubmission class, so its suppressions release. // -// On a won claim it runs executeAndFinalize and returns (true, execErr). -func (m *Manager) claimAndExecute(ctx context.Context, exec *config.PurchaseExecution) (claimed bool, err error) { - return m.claimAndExecuteWithin(ctx, exec, 0) -} - -// claimAndExecuteWithin is claimAndExecute with a due-row age cap: when maxAge -// is positive, a claimed row that was due more than maxAge ago is refused -// before submission (ErrDueRowTooOld) instead of bought (issue #712). The -// check runs on the claimed row, so a concurrent sweep that lost the claim +// When maxAge is positive, a claimed row that was due more than maxAge ago is +// refused before submission (ErrDueRowTooOld) instead of bought (issue #712). +// The check runs on the claimed row, so a concurrent sweep that lost the claim // never reaches it and the row is failed and reported exactly once. Only the -// process sweep passes a cap; the SQS execute_purchase path passes 0. +// process sweep passes a cap. +// +// On a won claim it runs executeAndFinalize and returns (true, execErr). func (m *Manager) claimAndExecuteWithin(ctx context.Context, exec *config.PurchaseExecution, maxAge time.Duration) (claimed bool, err error) { gated := exec.Status == "pending" || exec.Status == "notified" gateSawEmpty := len(exec.Recommendations) == 0 - // System-initiated: SQS/cron executor passes nil so transitioned_by = NULL. + // System-initiated: cron executor passes nil so transitioned_by = NULL. updated, claimErr := m.config.TransitionExecutionStatusPinned(ctx, exec.ExecutionID, []string{"approved", "pending", "notified"}, "running", nil, gateSawEmpty) if claimErr != nil { if errors.Is(claimErr, config.ErrNotFound) || errors.Is(claimErr, config.ErrExecutionNotInExpectedStatus) { @@ -316,7 +311,7 @@ func (b purchaseBudgets) perRecTimeout() time.Duration { // (completed / partially_completed / failed, classified by finalizeExecution // from the typed sentinel). The previous `!wasMultiAccount` save-skip left the // root in whatever pre-execution status it carried, which, now that -// claimAndExecute claims the root to "running" first (issue #1013), would strand +// claimAndExecuteWithin claims the root to "running" first (issue #1013), would strand // the root row in "running" until the reaper failed it. func (m *Manager) executeAndFinalize(ctx context.Context, exec *config.PurchaseExecution) error { // A disconnected caller must not cancel a purchase after the claim. @@ -501,7 +496,7 @@ func allRecsSafeToRedrive(exec *config.PurchaseExecution) bool { // // It is placed in executeAndFinalize rather than at the individual executors // because that function is the single funnel every executor reaches money -// through: claimAndExecute (SQS + cron), ApproveAndExecute (token + session +// through: claimAndExecuteWithin (cron), ApproveAndExecute (token + session // approve), fireOneDue (the scheduled pre-fire delay sweep) and claimAndRedrive // (the reaper). One gate at the funnel cannot be missed by a future executor // the way three copies at three call sites can. executePurchase has exactly one @@ -865,13 +860,13 @@ func (m *Manager) RecoverStrandedApprovals(ctx context.Context) (int, error) { } // executableByScheduler reports whether a pending or notified execution may be -// auto-executed by the cron sweep or an SQS execute_purchase message, without +// auto-executed by the cron sweep, without // an explicit human approval action (fail closed on money paths). // // Rules: // - web-submitted rows (Source == common.PurchaseSourceWeb, "cudly-web") must -// wait for the token-link approval path; the scheduler and SQS paths must -// never bypass that gate. +// wait for the token-link approval path; the scheduler must never +// bypass that gate. // - All other pending/notified rows require the owning plan to have // AutoPurchase=true. A plan-fetch error, or a missing plan, is propagated // so the caller can fail closed rather than defaulting to "execute". A @@ -1007,8 +1002,8 @@ func (m *Manager) processOneExecution(ctx context.Context, exec config.PurchaseE // Atomically claim the row before executing (issue #1013). Overlapping // cron ticks (a tick that runs longer than the interval, EventBridge - // duplicate/overlapping deliveries, or cron racing the SQS path) would - // otherwise both execute the same due row. claimAndExecute CASes the row + // duplicate/overlapping deliveries, or cron racing a manual run) would + // otherwise both execute the same due row. claimAndExecuteWithin CASes the row // to "running" and only the winner runs; a lost claim is skipped without // re-executing. claimed, execErr := m.claimAndExecuteWithin(ctx, &exec, m.dueRowMaxAge()) diff --git a/internal/purchase/messages.go b/internal/purchase/messages.go deleted file mode 100644 index df3b6d7a9..000000000 --- a/internal/purchase/messages.go +++ /dev/null @@ -1,131 +0,0 @@ -package purchase - -import ( - "context" - "encoding/json" - "errors" - "fmt" - - "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" - "github.com/LeanerCloud/cloud-commitments-platform/internal/config" -) - -// MessageType defines the types of async messages that can be processed. -type MessageType string - -const ( - // MessageTypeExecutePurchase triggers execution of a scheduled purchase. - MessageTypeExecutePurchase MessageType = "execute_purchase" - // MessageTypeSendNotification sends a notification for upcoming purchase. - MessageTypeSendNotification MessageType = "send_notification" -) - -// AsyncMessage represents an SQS message for async purchase processing. -type AsyncMessage struct { - Type MessageType `json:"type"` - ExecutionID string `json:"execution_id,omitempty"` - PlanID string `json:"plan_id,omitempty"` -} - -// ProcessMessage handles SQS messages for async purchase processing. -// Supported message types: -// - execute_purchase: Execute a scheduled purchase by execution_id -// - send_notification: Send notification for upcoming purchases -func (m *Manager) ProcessMessage(ctx context.Context, body string) error { - logging.Debug("Processing async message") - - // Parse the message - var msg AsyncMessage - if err := json.Unmarshal([]byte(body), &msg); err != nil { - // If not valid JSON, log and skip (don't fail the queue) - logging.Warnf("Invalid message format (not JSON), skipping: %v", err) - return nil - } - - switch msg.Type { - case MessageTypeExecutePurchase: - return m.handleExecutePurchase(ctx, &msg) - case MessageTypeSendNotification: - _, err := m.SendUpcomingPurchaseNotifications(ctx) - return err - default: - logging.Warnf("Unknown message type: %s, skipping", msg.Type) - return nil - } -} - -// handleExecutePurchase processes an execute_purchase message. -// checkAutoExecuteGate enforces the AutoPurchase and source gate for -// pending/notified rows on the SQS execute_purchase path. Extracted from -// handleExecutePurchase to keep that function under the gocyclo:10 limit. -// Returns nil when execution may proceed; a non-nil error means the message -// should be dead-lettered (the row is not eligible for auto-execution). -// -// "approved" rows bypass the gate: they have already passed the human -// approval path and are always eligible for execution. -func (m *Manager) checkAutoExecuteGate(ctx context.Context, execution *config.PurchaseExecution) error { - if execution.Status != "pending" && execution.Status != "notified" { - return nil - } - eligible, err := m.executableByScheduler(ctx, execution) - if err != nil { - return fmt.Errorf("AutoPurchase gate check failed for execution %s: %w", execution.ExecutionID, err) - } - if !eligible { - return fmt.Errorf("execution %s (status=%s source=%q) is not eligible for auto-execution: AutoPurchase=false, kill switch off, source=web, 4-eyes on (it needs a human approval in the approval queue), or recommendation-bearing row not yet notified - approve it in the approval queue", - execution.ExecutionID, execution.Status, execution.Source) - } - return nil -} - -func (m *Manager) handleExecutePurchase(ctx context.Context, msg *AsyncMessage) error { - if msg.ExecutionID == "" { - return fmt.Errorf("execution_id required for execute_purchase message") - } - - execution, err := m.config.GetExecutionByID(ctx, msg.ExecutionID) - if errors.Is(err, config.ErrNotFound) { - return fmt.Errorf("execution not found: %s", msg.ExecutionID) - } - if err != nil { - return fmt.Errorf("failed to get execution %s: %w", msg.ExecutionID, err) - } - - logging.Infof("Executing purchase from async message: %s", msg.ExecutionID) - - // AutoPurchase gate: pending/notified rows require AutoPurchase=true on the - // owning plan; web-sourced rows must use the token-link approval path. - // "approved" rows bypass the gate (already human-approved). Fail closed. - if err := m.checkAutoExecuteGate(ctx, execution); err != nil { - return err - } - - // Atomically claim the row before touching the cloud (issue #1013). SQS - // delivery is at-least-once: a redelivered execute_purchase message (slow - // purchase whose visibility timeout expired, partial-batch replay, operator - // re-drive) would otherwise pass the old non-atomic status check and execute - // the same row a second time concurrently. claimAndExecute CASes - // approved/pending/notified -> running and only proceeds on a won claim; a - // lost claim is a benign duplicate that we ack (return nil) without - // re-executing. - claimed, execErr := m.claimAndExecute(ctx, execution) - if !claimed { - // Either a benign CAS race-loss (execErr == nil — ack the duplicate) or - // a real DB error during the claim (execErr != nil — surface it so the - // message is redelivered, since nothing was executed). - if execErr != nil { - return fmt.Errorf("failed to claim execution %s: %w", msg.ExecutionID, execErr) - } - return nil - } - - // A multi-account run where at least one account committed must be ACKed, - // not redelivered (issue #1014): redelivery would re-run the fan-out and, - // absent the per-account idempotency key (#1012), double-buy the accounts - // that already succeeded. errAllAccountsFailed (nothing committed) and - // single-account errors fall through and are returned so SQS redelivers. - if isMultiAccountAckable(execErr) { - return nil - } - return execErr -} diff --git a/internal/purchase/messages_test.go b/internal/purchase/messages_test.go deleted file mode 100644 index ac1ee4e62..000000000 --- a/internal/purchase/messages_test.go +++ /dev/null @@ -1,187 +0,0 @@ -package purchase - -import ( - "context" - "fmt" - "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" -) - -func TestManager_ProcessMessage(t *testing.T) { - ctx := context.Background() - - t.Run("invalid JSON message", func(t *testing.T) { - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - // Invalid JSON should be skipped without error - err := manager.ProcessMessage(ctx, "plain text") - assert.NoError(t, err) - }) - - t.Run("unknown message type", func(t *testing.T) { - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - err := manager.ProcessMessage(ctx, `{"type": "unknown_type"}`) - assert.NoError(t, err) - }) - - t.Run("execute_purchase missing execution_id", func(t *testing.T) { - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - err := manager.ProcessMessage(ctx, `{"type": "execute_purchase"}`) - assert.Error(t, err) - assert.Contains(t, err.Error(), "execution_id required") - }) - - t.Run("send_notification success", func(t *testing.T) { - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return([]config.PurchasePlan{}, nil) - - err := manager.ProcessMessage(ctx, `{"type": "send_notification"}`) - assert.NoError(t, err) - mockStore.AssertExpectations(t) - }) - - t.Run("execute_purchase execution not found", func(t *testing.T) { - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - mockStore.On("GetExecutionByID", ctx, "exec-notfound").Return(nil, fmt.Errorf("%w: execution exec-notfound", config.ErrNotFound)) - - err := manager.ProcessMessage(ctx, `{"type": "execute_purchase", "execution_id": "exec-notfound"}`) - assert.Error(t, err) - assert.Contains(t, err.Error(), "execution not found") - }) - - // ─── F1 regression: AutoPurchase gate on SQS execute_purchase path ──────── - - t.Run("execute_purchase pending web-source rejected (F1)", func(t *testing.T) { - // A web-submitted pending row must not be auto-executed via SQS; - // it must wait for the token-link approval path. - mockStore := new(MockConfigStore) - manager := &Manager{config: mockStore, dashboardURL: "https://example.com"} - execution := &config.PurchaseExecution{ - ExecutionID: "exec-web", - PlanID: "plan-1", - Status: "pending", - Source: common.PurchaseSourceWeb, - } - mockStore.On("GetExecutionByID", ctx, "exec-web").Return(execution, nil) - // Sentinel: web rows must short-circuit BEFORE the AutoPurchase plan - // fetch. Return AutoPurchase=true (the dangerous case) and flag the - // fetch; the default GetPurchasePlan stub doesn't record the call, so - // AssertNotCalled alone can't catch a bypass of the "cudly-web" gate. - planFetched := false - mockStore.GetPurchasePlanFn = func(_ context.Context, planID string) (*config.PurchasePlan, error) { - planFetched = true - return &config.PurchasePlan{ID: planID, AutoPurchase: true}, nil - } - - err := manager.ProcessMessage(ctx, `{"type": "execute_purchase", "execution_id": "exec-web"}`) - assert.Error(t, err) - assert.Contains(t, err.Error(), "not eligible for auto-execution") - assert.False(t, planFetched, - "web rows must short-circuit before the AutoPurchase plan fetch on the SQS path") - mockStore.AssertNotCalled(t, "TransitionExecutionStatus", - mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) - }) - - t.Run("execute_purchase pending AutoPurchase=false rejected (F1)", func(t *testing.T) { - // A pending row under a plan with AutoPurchase=false must not be - // auto-executed via SQS. - mockStore := new(MockConfigStore) - manager := &Manager{config: mockStore, dashboardURL: "https://example.com"} - execution := &config.PurchaseExecution{ - ExecutionID: "exec-noauto", - PlanID: "plan-noauto", - Status: "pending", - } - plan := &config.PurchasePlan{ID: "plan-noauto", AutoPurchase: false} - mockStore.On("GetExecutionByID", ctx, "exec-noauto").Return(execution, nil) - mockStore.On("GetPurchasePlan", ctx, "plan-noauto").Return(plan, nil) - // TransitionExecutionStatus must NOT be called - - err := manager.ProcessMessage(ctx, `{"type": "execute_purchase", "execution_id": "exec-noauto"}`) - assert.Error(t, err) - assert.Contains(t, err.Error(), "not eligible for auto-execution") - mockStore.AssertNotCalled(t, "TransitionExecutionStatus", - mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) - mockStore.AssertExpectations(t) - }) - - t.Run("execute_purchase wrong status", func(t *testing.T) { - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", - } - execution := &config.PurchaseExecution{ - ExecutionID: "exec-123", - Status: "canceled", - } - mockStore.On("GetExecutionByID", ctx, "exec-123").Return(execution, nil) - // The claim CAS rejects a non-executable status (issue #1013): the row - // is "canceled", not in [approved,pending,notified], so the CAS loses - // and returns ErrExecutionNotInExpectedStatus — a benign skip that the - // handler acks without error. - mockStore.On("TransitionExecutionStatus", ctx, "exec-123", - []string{"approved", "pending", "notified"}, "running", (*string)(nil)). - Return(nil, fmt.Errorf("%w: canceled", config.ErrExecutionNotInExpectedStatus)) - - err := manager.ProcessMessage(ctx, `{"type": "execute_purchase", "execution_id": "exec-123"}`) - // Should skip without error when status is not executable - assert.NoError(t, err) - }) -} - -// #1771: an SQS execute_purchase for an auto row is refused while 4-eyes is -// on, with an error that names the reason, and nothing is claimed or bought. -func TestCheckAutoExecuteGate_FourEyesOnRefusesNotifiedRow(t *testing.T) { - ctx := context.Background() - store := new(MockConfigStore) // TransitionExecutionStatus is not mocked: a claim would panic - assertMocksOnCleanup(t, store) - cfg := autoPurchaseOnCfg() - cfg.RequireDifferentApprover = true - store.On("GetPurchasePlan", ctx, "plan-gate").Return(&config.PurchasePlan{ID: "plan-gate", AutoPurchase: true, MaxStepCommitment: f64(1000)}, nil) - store.On("GetGlobalConfig", ctx).Return(cfg, nil) - manager := &Manager{config: store} - exec := dueRecExec("notified", upfrontRec(10)) - - err := manager.checkAutoExecuteGate(ctx, &exec) - - require.Error(t, err) - assert.Contains(t, err.Error(), "4-eyes") - store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) -} diff --git a/internal/purchase/redshift_gate_integration_test.go b/internal/purchase/redshift_gate_integration_test.go index e0f699d33..08fbb63d5 100644 --- a/internal/purchase/redshift_gate_integration_test.go +++ b/internal/purchase/redshift_gate_integration_test.go @@ -93,9 +93,9 @@ func TestRedshiftDispatchGate_ConflictRollsBackStamp(t *testing.T) { open := f.redshiftRow(ctx, t, "running") _, err := f.pool.Exec(ctx, `UPDATE purchase_executions SET first_dispatch_at = now() WHERE execution_id = $1`, open.ExecutionID) require.NoError(t, err) - refused := f.redshiftRow(ctx, t, "approved") + refused := f.redshiftRow(ctx, t, "notified") - claimed, _ := f.mgr.claimAndExecute(ctx, rowStatusCopy(refused)) + claimed, _ := f.mgr.claimAndExecuteWithin(ctx, rowStatusCopy(refused), 0) require.True(t, claimed) assert.Zero(t, f.bought.Load(), "a refused lineage never reaches the provider") @@ -113,7 +113,7 @@ func TestRedshiftDispatchGate_TwoLineagesRaceOnlyOneBuys(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() f := newRedshiftGateFixture(ctx, t) - f.raceOnlyOneBuys(ctx, t, f.redshiftRow(ctx, t, "approved"), f.redshiftRow(ctx, t, "approved")) + f.raceOnlyOneBuys(ctx, t, f.redshiftRow(ctx, t, "notified"), f.redshiftRow(ctx, t, "notified")) } // A root row (key R) and a per-account successor (key R:acct) share one @@ -124,8 +124,8 @@ func TestRedshiftDispatchGate_RootAndAccountSuccessorsOfOneFamilyDoNotBothBuy(t f := newRedshiftGateFixture(ctx, t) family := uuid.New().String() f.raceOnlyOneBuys(ctx, t, - f.redshiftRowKeyed(ctx, t, "approved", family, true), - f.redshiftRowKeyed(ctx, t, "approved", family+":"+f.accounts[0].ID, false)) + f.redshiftRowKeyed(ctx, t, "notified", family, true), + f.redshiftRowKeyed(ctx, t, "notified", family+":"+f.accounts[0].ID, false)) } func (f *redshiftGateFixture) raceOnlyOneBuys(ctx context.Context, t *testing.T, rows ...*config.PurchaseExecution) { @@ -158,7 +158,7 @@ func (f *redshiftGateFixture) raceOnlyOneBuys(ctx context.Context, t *testing.T, go func() { defer wg.Done() <-start - _, _ = f.mgr.claimAndExecute(ctx, rowStatusCopy(row)) + _, _ = f.mgr.claimAndExecuteWithin(ctx, rowStatusCopy(row), 0) }() } close(start) From 55dc44ec30c00bd7993467499c9ff02e305537f6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 17:59:31 +0100 Subject: [PATCH 12/14] docs: drop SQS wording and record the removal Reword comments that described the removed SQS path, add the CHANGELOG Removed entry (the Lambda rejects aws:sqs events and SQS_REPORT_BATCH_ITEM_FAILURES is no longer read), and note the now unreachable "approved" entry of the claim funnel's from-set in known-issues.md as a follow-up. Refs #399 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 +++ internal/api/types.go | 2 +- ...postgres_no_submission_integration_test.go | 4 +- internal/purchase/approvals.go | 43 ++++++++----------- .../cancel_scheduled_integration_test.go | 2 +- internal/purchase/claim.go | 6 +-- internal/purchase/claimed_row_check_test.go | 2 +- internal/purchase/due_age_cap_test.go | 2 +- internal/purchase/execution.go | 4 +- .../purchase/failure_notify_writers_test.go | 2 +- internal/purchase/manager_test.go | 2 +- .../purchase/money_path_regression_test.go | 29 ++++++------- .../plan_step_atomic_integration_test.go | 2 +- known-issues.md | 9 ++++ 14 files changed, 61 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11ebb7d4b..09a0bc882 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,6 +72,11 @@ are the full record until the next release. ### Removed +- The async SQS purchase-execution path (issue #399). Nothing produced SQS + messages, so the Lambda no longer accepts an `aws:sqs` event (it now fails + with "unrecognized Lambda event shape"), and the + `SQS_REPORT_BATCH_ITEM_FAILURES` environment variable is no longer read. + - The CloudFormation install stack (`cloudformation/stacks/CUDly`). It still created DynamoDB tables and set no `DB_HOST`, so the Postgres-only server it deployed failed at startup and the stack could not serve any request or diff --git a/internal/api/types.go b/internal/api/types.go index 4729ba607..ed0c94519 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -141,7 +141,7 @@ type BreakdownValue struct { // // `transitionedBy` (ApproveAndExecute) is the session user's UUID stamped // onto purchase_executions.transitioned_by for human-initiated approvals; -// pass nil for token/SQS/system flows so transitioned_by = NULL (issue #1009). +// pass nil for token/system flows so transitioned_by = NULL (issue #1009). // // ApproveExecution, ApproveAndExecute and RunPlannedPurchaseNow return the // raw revocation token minted on a successful execute (issue #103: only its diff --git a/internal/config/store_postgres_no_submission_integration_test.go b/internal/config/store_postgres_no_submission_integration_test.go index 47de56539..af67e6fcf 100644 --- a/internal/config/store_postgres_no_submission_integration_test.go +++ b/internal/config/store_postgres_no_submission_integration_test.go @@ -104,7 +104,7 @@ func TestNoSubmission_NonFailedWriteClearsProof(t *testing.T) { // Replays the approve-path sequence at the store level (approvals.go:392-421): // the approve path keeps a stale copy of the approved row; a concurrent -// claimAndExecute fails the row before submission (proof true); the approve +// claimAndExecuteWithin fails the row before submission (proof true); the approve // path's audit upsert puts the row back to approved; the approve path then // calls the provider and the row ends failed with outcome unknown (flag // false). The proof must not survive on a row that reached the provider. @@ -114,7 +114,7 @@ func TestNoSubmission_StaleApproveUpsertDoesNotKeepProofOnSubmittedRow(t *testin exec := newNoSubmissionExec(ctx, t, store, "approved") staleApproved := *exec - exec.Status = "failed" // claimAndExecute: refused before submission + exec.Status = "failed" // claimAndExecuteWithin: refused before submission exec.NoSubmission = true require.NoError(t, store.SavePurchaseExecution(ctx, exec)) diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index 87439ae77..391060bb2 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -34,20 +34,19 @@ const runNowSkipReason = "run-now" const directExecuteSkipReason = "direct-execute permission" // ApproveExecution is the token-authenticated approve entry point used by -// the legacy email-link flow and the SQS approve worker. After validating +// the legacy email-link flow. After validating // the approval token it hands off to the ApproveAndExecute funnel // (transitionApproveAndExecute), which performs the atomic status transition // with the token pinned inside it, runs the AWS purchase synchronously, and // mints the fresh revocation token returned here. // // actor carries the email of the operator who triggered the approval -// (session-authed click on the HTTP path; verified actor_email on the SQS -// path) so it can be stamped onto ApprovedBy. Empty actor is recorded as +// (the session-authed click on the HTTP path) so it can be stamped onto ApprovedBy. Empty actor is recorded as // NULL — the column is nullable TEXT and we don't want to claim "approved // by nobody". // // check is the caller's ClaimedRowCheck, run against the claimed row before -// any provider call (issue #415); the SQS approve worker passes nil. +// any provider call (issue #415); the token path passes nil. // // Returns the same row and token as ApproveAndExecute (issue #532): the row // carries the run's final status, also on an execution error, and is nil when @@ -84,7 +83,7 @@ func (m *Manager) ApproveExecution(ctx context.Context, executionID, token, acto return nil, "", checkErr } - // Token/SQS path: no authenticated session UUID is available, so the + // Token path: no authenticated session UUID is available, so the // transition is recorded as system-initiated (transitioned_by = NULL). // The token check above read a snapshot; the claim pins the same token // inside its status CAS so a re-notification that rotated it since cannot @@ -186,16 +185,14 @@ func OrphanExecutionError(execution *config.PurchaseExecution) error { // // - approvePurchaseViaSession (session-authed dashboard approve) // - directExecutePurchase (execute_mode="direct" -- issue #289) -// - ApproveExecution (email-token deep-link approve AND the SQS async -// approve worker, both of which funnel into ApproveAndExecute) +// - ApproveExecution (email-token deep-link approve, which funnels into +// ApproveAndExecute) // // Adversarial review of PR #1500 found that requireDifferentApprover in // internal/api was wired into only 2 of these 4 entry points, leaving // execute_mode="direct" (HIGH: the requester always equals the creator on -// this path, so this is unconditional self-authorization) and the SQS -// approve worker (MEDIUM: token + actor_email verification never compared -// the actor against the creator) able to bypass dual control entirely. This -// method closes both gaps by running at the one place all four paths share. +// this path, so this is unconditional self-authorization) able to bypass +// dual control entirely. This method closes that gap by running at the one place all four paths share. // // Identity comparison has two tiers, checked in order of precision: // @@ -214,11 +211,10 @@ func OrphanExecutionError(execution *config.PurchaseExecution) error { // email domain entirely for these two callers and cannot be fooled by an // empty or substituted email string. // 2. actorEmail: used only when actorUserID is nil -- the token -// (approveViaToken) and SQS (handleApproveMessage) callers into -// ApproveExecution, which always pass transitionedBy=nil (see -// ApproveExecution's own doc comment), so the RBAC/contact-email- -// verified actor identity from authorizeApprovalAction / -// verifyAsyncApprovalActor is the only signal available there. +// (approveViaToken) caller into ApproveExecution, which always passes +// transitionedBy=nil (see ApproveExecution's own doc comment), so the +// RBAC/contact-email-verified actor identity from +// authorizeApprovalAction is the only signal available there. // execution.CreatedByUserID is a UUID; internal/purchase cannot resolve // it through internal/auth directly (internal/auth already imports // internal/config, so the reverse import would cycle), so @@ -299,7 +295,7 @@ func (m *Manager) checkDifferentApprover(ctx context.Context, executionID string return nil } - // Tier 2: no actor UUID available (token/SQS callers) -- fall back to + // Tier 2: no actor UUID available (token callers) -- fall back to // resolving and comparing emails. actorEmail = strings.TrimSpace(actorEmail) if actorEmail == "" { @@ -331,7 +327,7 @@ func (m *Manager) checkDifferentApprover(ctx context.Context, executionID string // synchronously. Callers MUST have already authorized the actor — this // method does no RBAC or token validation; it is shared between: // -// - ApproveExecution (token/SQS path: token validated by the caller and +// - ApproveExecution (token path: token validated by the caller and // pinned inside the CAS; it calls transitionApproveAndExecute directly) // - approvePurchaseViaSession (session path: RBAC validated by the caller) // - directExecutePurchase (execute_mode="direct": RBAC validated by the caller) @@ -370,7 +366,7 @@ func (m *Manager) DirectExecute(ctx context.Context, executionID, actor string, // was told it ran. // // Sharing transitionApproveAndExecute's CAS with ApproveAndExecute and -// claimAndExecute is also what makes this idempotent: a second "Run now" +// claimAndExecuteWithin is also what makes this idempotent: a second "Run now" // 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. @@ -417,7 +413,7 @@ func (m *Manager) transitionApproveAndExecute(ctx context.Context, executionID, // Universal 4-eyes gate (issue #1005 / PR #1500 adversarial review): runs // before any state mutation so every caller -- session approve, direct - // execute, run-now, token approve, SQS approve -- is covered by one + // execute, run-now, token approve -- is covered by one // policy check. transitionedBy doubles as the actor's own UUID for this // check when the caller has one (session-based callers); see // enforceFourEyesPolicy's doc comment for the full rationale. @@ -427,11 +423,11 @@ func (m *Manager) transitionApproveAndExecute(ctx context.Context, executionID, } // transitionedBy carries the session user's UUID for human-initiated - // approvals (stamped onto transitioned_by); it is nil for token/SQS/system + // approvals (stamped onto transitioned_by); it is nil for token/system // flows so transitioned_by = NULL on those hops. The human-readable actor // email is recorded separately onto approved_by (below). // One CAS straight to "running": winning it is the claim, so no other - // executor (SQS, cron, re-drive, scheduler) can run this row, and there is + // executor (cron, re-drive, scheduler) can run this row, and there is // no window where the row sits "approved" with a stale copy in flight // (issue #829). A lost CAS surfaces ErrExecutionNotInExpectedStatus. updated, err := m.claimAndCheck(ctx, executionID, fromStatuses, transitionedBy, approvalToken, check, t0) @@ -504,8 +500,7 @@ func (m *Manager) runClaimedAndMint(ctx context.Context, updated *config.Purchas // CancelExecution cancels a pending execution. actor carries the email of // the session-authenticated user who clicked cancel; verified by the -// caller (HTTP path: authorizeApprovalAction; SQS path: -// verifyAsyncApprovalActor) before reaching here. Same empty-actor +// caller (HTTP path: authorizeApprovalAction) before reaching here. Same empty-actor // rationale as ApproveExecution. // // Concurrency: CancelExecutionAtomicTokenPinned uses a conditional UPDATE diff --git a/internal/purchase/cancel_scheduled_integration_test.go b/internal/purchase/cancel_scheduled_integration_test.go index 6aaa4311c..396f95df3 100644 --- a/internal/purchase/cancel_scheduled_integration_test.go +++ b/internal/purchase/cancel_scheduled_integration_test.go @@ -13,7 +13,7 @@ import ( "github.com/stretchr/testify/require" ) -// Real PostgreSQL. Issue #73: the token/SQS cancel of a scheduled row used to +// Real PostgreSQL. Issue #73: the token cancel of a scheduled row used to // reach CancelExecutionAtomic (status IN pending/notified), match zero rows and // report a "concurrent operation" that never happened. diff --git a/internal/purchase/claim.go b/internal/purchase/claim.go index ad3ed7bf4..473599c5c 100644 --- a/internal/purchase/claim.go +++ b/internal/purchase/claim.go @@ -27,12 +27,12 @@ var ErrClaimedRowRefused = errors.New("purchase refused after claim: the claimed // earlier check ran on a snapshot read before the CAS, so a repricing or a // recommendation attach landing in between would otherwise be bought // unchecked (issue #415). A nil check skips the re-check; only callers with -// no pre-CAS Constraints check (the SQS approve worker) pass nil. +// no pre-CAS Constraints check (the token approve) pass nil. type ClaimedRowCheck func(ctx context.Context, claimed *config.PurchaseExecution) error // claimRunning is the single claim primitive: an atomic CAS from one of from // to "running". Winning it is the right to call the provider; every executor -// (approve, scheduler, SQS, cron, re-drive) claims before any provider call. +// (approve, scheduler, cron, re-drive) claims before any provider call. // A lost CAS returns an error satisfying // errors.Is(err, config.ErrExecutionNotInExpectedStatus) or config.ErrNotFound. // actor is the transitioned_by user UUID, nil for system-initiated claims. @@ -41,7 +41,7 @@ func (m *Manager) claimRunning(ctx context.Context, executionID string, from []s } // claimRunningForApprove is claimRunning for transitionApproveAndExecute. A -// non-empty approvalToken (the email-link and SQS token approve) is pinned in +// non-empty approvalToken (the email-link token approve) is pinned in // the same UPDATE as the claim: the row's stored token hash must still match // it and be unexpired, so a re-notification that rotated the token after // ApproveExecution validated its snapshot refuses the claim with diff --git a/internal/purchase/claimed_row_check_test.go b/internal/purchase/claimed_row_check_test.go index 060c45b47..ec3fdaeb3 100644 --- a/internal/purchase/claimed_row_check_test.go +++ b/internal/purchase/claimed_row_check_test.go @@ -233,7 +233,7 @@ func TestClaimedRowCheck_PlanRowReleasesSuppressions(t *testing.T) { store.AssertCalled(t, "DeleteSuppressionsByExecutionTx", mock.Anything, mock.Anything, planRow.ExecutionID) } -// A nil check (the SQS approve worker, which has no session Constraints) +// A nil check (the token approve, which has no session Constraints) // keeps the previous behavior. func TestClaimedRowCheck_NilCheckSkipsRecheck(t *testing.T) { ep := claimEntryPoints()[3] diff --git a/internal/purchase/due_age_cap_test.go b/internal/purchase/due_age_cap_test.go index e29099a42..9750e209c 100644 --- a/internal/purchase/due_age_cap_test.go +++ b/internal/purchase/due_age_cap_test.go @@ -211,7 +211,7 @@ func TestProcessScheduledPurchases_CapNeverTouchesRowsAwaitingApproval(t *testin assert.Equal(t, ProcessResult{}, *result) } -// The SQS execute_purchase path shares the claim funnel but passes no cap. +// Callers that pass no cap share the claim funnel but never refuse a stale row. func TestClaimedRowRefusal_NoCapWithoutMaxAge(t *testing.T) { ancient := &config.PurchaseExecution{ScheduledDate: time.Now().Add(-365 * 24 * time.Hour)} require.NoError(t, (&Manager{}).claimedRowRefusal(context.Background(), ancient, false, 0)) diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index ecf287ae7..950f19bb0 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -206,8 +206,8 @@ func anyRecPurchased(recs []config.RecommendationRecord) bool { // failed (issue #1014). It is the multi-account analog of partialPurchaseError: // the executor entry points must NOT treat this as a flat failure — the // per-account rows already own their authoritative status (partially_completed / -// completed / failed) and real commitments exist, so an SQS/cron caller must ACK -// the message (not redeliver) to avoid re-running the fan-out and double-buying +// completed / failed) and real commitments exist, so the cron sweep must count it as +// executed (not retry it) to avoid re-running the fan-out and double-buying // the accounts that already succeeded (which #1012's stable key would otherwise // dedupe, but the contract should not depend on that second line of defense). type multiAccountPartialError struct { diff --git a/internal/purchase/failure_notify_writers_test.go b/internal/purchase/failure_notify_writers_test.go index b0b01af6e..de93bb68b 100644 --- a/internal/purchase/failure_notify_writers_test.go +++ b/internal/purchase/failure_notify_writers_test.go @@ -107,7 +107,7 @@ func TestNotifyRootFailure_FanOutRootLogsMarkerWithoutEmail(t *testing.T) { }) } -// fanOutRun drives a two-account plan through the SQS executor. Account +// fanOutRun drives a two-account plan through the scheduled sweep. Account // "acct-ok" buys; the other account's behavior is set by the caller through the // credential store and the save hook. It returns the root's saved status and // every purchase-failed email. diff --git a/internal/purchase/manager_test.go b/internal/purchase/manager_test.go index 299d4ef78..e468b12f5 100644 --- a/internal/purchase/manager_test.go +++ b/internal/purchase/manager_test.go @@ -208,7 +208,7 @@ func TestManager_ProcessScheduledPurchases_DuePurchase(t *testing.T) { }, } - // claimAndExecute CASes the due row to "running" before executing (issue #1013). + // claimAndExecuteWithin CASes the due row to "running" before executing (issue #1013). claimedExec := executions[0] claimedExec.Status = "running" mockStore.On("GetStaleApprovedExecutions", ctx, mock.Anything).Return([]config.PurchaseExecution{}, nil) diff --git a/internal/purchase/money_path_regression_test.go b/internal/purchase/money_path_regression_test.go index 9011555d9..0d6b46242 100644 --- a/internal/purchase/money_path_regression_test.go +++ b/internal/purchase/money_path_regression_test.go @@ -301,13 +301,13 @@ func TestMultiAccountSeedsStablePerAccountKey(t *testing.T) { // TestScheduledSweepOverlapDoesNotDoubleExecute is the issue #1013 (C2) regression // guard: overlapping scheduled sweeps must NOT execute the same row a -// second time. The first delivery wins the CAS to "running"; the redelivery -// loses the CAS (the row is no longer in an executable state) and is acked -// without touching the cloud. +// second time. The first sweep wins the CAS to "running"; the second loses the +// CAS (the row is no longer in an executable state) and skips without touching +// the cloud. // -// Pre-fix, handleExecutePurchase only checked status in (approved,pending) with -// no atomic transition, so both deliveries would execute. This test asserts the -// purchase runs exactly once and the second delivery is a benign no-op. +// Pre-fix, the executor only checked status in (approved,pending) with no +// atomic transition, so both sweeps would execute. This test asserts the +// purchase runs exactly once and the second sweep is a benign no-op. func TestScheduledSweepOverlapDoesNotDoubleExecute(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) @@ -334,9 +334,8 @@ func TestScheduledSweepOverlapDoesNotDoubleExecute(t *testing.T) { } } - // Both deliveries read the row as "pending" from the DB (at-least-once SQS: - // pre-fix nothing CASes it to running before the cloud call, so a redelivery - // re-reads a still-claimable row). Each GetExecutionByID returns a FRESH + // Both deliveries read the row as "pending" from the DB (pre-fix nothing CASes it to running + // before the cloud call, so an overlapping sweep re-reads a still-claimable row). Each GetExecutionByID returns a FRESH // pending copy, faithfully modeling the real double-delivery scenario. mockStore.On("GetGlobalConfig", ctx).Return(autoPurchaseOnCfg(), nil).Once() mockStore.On("GetGlobalConfig", mock.Anything).Return(autoPurchaseOnCfg(), nil).Maybe() @@ -391,8 +390,8 @@ func TestScheduledSweepOverlapDoesNotDoubleExecute(t *testing.T) { // TestMultiAccountPartialSuccessIsAcked is the issue #1014 regression guard: a // multi-account run where at least one account committed must NOT surface as a -// flat failure to the SQS handler (which would trigger redelivery and a -// double-buy). The handler must return nil (ack) when committed >= 1. +// flat failure to the sweep (which would retry and double-buy). The sweep +// must count it Executed when committed >= 1. func TestMultiAccountPartialSuccessIsAcked(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) @@ -487,10 +486,10 @@ func TestMultiAccountPartialSuccessIsAcked(t *testing.T) { // place where it returns committed==true together with an error -- the cloud // purchase succeeded but the per-account SavePurchaseExecution call failed // (execution.go:269-270, "AUDIT LOSS: failed to save execution record"). That -// return value is what stops the SQS message from being redelivered and the +// return value is what stops the row from being retried and the // commitment from being bought twice: executeMultiAccount folds a committed // account with an error into *multiAccountPartialError, which -// handleExecutePurchase acks rather than surfacing as a flat failure. +// the sweep counts as executed rather than surfacing as a flat failure. // // acct-bad deliberately fails credential resolution (committed=false, like // TestMultiAccountPartialSuccessIsAcked's acct-bad) so acct-unsaved is the @@ -498,8 +497,8 @@ func TestMultiAccountPartialSuccessIsAcked(t *testing.T) { // the AUDIT LOSS return to "return false, ..." (reading it as "we failed, // report failure"), committed would drop to 0 across both accounts and the // aggregator would return errAllAccountsFailed instead of -// *multiAccountPartialError, which handleExecutePurchase does NOT ack -- -// this test would then fail on require.NoError. Every other +// *multiAccountPartialError, which the sweep does NOT count as executed -- +// this test would then fail on the Executed assertion. Every other // SavePurchaseExecutionFn stub in this package returns nil on a committed // account, and the only stub that errors (execution_test.go:923) drives the // credential-failure AUDIT LOSS site at execution.go:300, not this one. diff --git a/internal/purchase/plan_step_atomic_integration_test.go b/internal/purchase/plan_step_atomic_integration_test.go index 4d3e993e5..ba39f672d 100644 --- a/internal/purchase/plan_step_atomic_integration_test.go +++ b/internal/purchase/plan_step_atomic_integration_test.go @@ -42,7 +42,7 @@ func stepRowsInDB(ctx context.Context, t *testing.T, store *config.PostgresStore return rows } -// A4: concurrent ticks (SQS redelivery, cron overlap, scheduler retries) must +// A4: concurrent ticks (cron overlap, scheduler retries) must // leave exactly one row for the step. Without the plan lock every tick reads // "nothing there" and inserts its own. func TestGetOrCreateExecution_ConcurrentTicksMintOneRow(t *testing.T) { diff --git a/known-issues.md b/known-issues.md index dcdef5610..12db50c2c 100644 --- a/known-issues.md +++ b/known-issues.md @@ -5,6 +5,15 @@ action. Resolved items are moved to the Resolved section at the bottom. ## Outstanding +### `claimAndExecuteWithin` still claims from `approved` (issue #399 follow-up) + +The claim funnel's from-set is `approved, pending, notified`. With the SQS +execute path gone, no executor reaches it with an `approved` row (the +approve paths claim `pending, notified` and stranded `approved` rows go +through the re-drive), so the `approved` entry is unreachable in production. +Left in place by issue #399 to keep that change a pure removal; dropping it +needs its own change and test review. + ### Existing Azure deployments carry a tenant-wide reservation grant (issue #1545) Until issue #1545 was fixed, `arm/CUDly-CrossSubscription/template.json` From fe39d835f63911813b94a68ecdbb3aa4875d48fe Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 20:06:09 +0100 Subject: [PATCH 13/14] docs: drop SQS-era delivery wording from comments and test names Refs #399 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_history.go | 2 +- internal/purchase/approvals.go | 11 ++--- internal/purchase/approvals_test.go | 41 ++++++++++--------- internal/purchase/execution.go | 4 +- internal/purchase/manager.go | 14 +++---- .../purchase/money_path_regression_test.go | 12 +++--- known-issues.md | 4 +- 7 files changed, 45 insertions(+), 43 deletions(-) diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 72c06674a..80e046e93 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -515,7 +515,7 @@ func annotateInFlightOrAuditGapRow(row *config.PurchaseHistoryRecord, exec confi // exact session-authed click attribution. // 2. approver — the notification inbox that received the cancel token; // authoritative accountable party but not necessarily the clicker. -// Used on legacy token-only paths (async workers, old email clicks). +// Used on legacy token-only paths (old email clicks). func annotateCancelled(row *config.PurchaseHistoryRecord, exec config.PurchaseExecution, approver string) { if exec.CancelledBy != nil && *exec.CancelledBy != "" { row.Approver = *exec.CancelledBy diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index 391060bb2..d7937e22d 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -41,9 +41,9 @@ const directExecuteSkipReason = "direct-execute permission" // mints the fresh revocation token returned here. // // actor carries the email of the operator who triggered the approval -// (the session-authed click on the HTTP path) so it can be stamped onto ApprovedBy. Empty actor is recorded as -// NULL — the column is nullable TEXT and we don't want to claim "approved -// by nobody". +// (the session-authed click on the HTTP path) so it can be stamped onto +// ApprovedBy. Empty actor is recorded as NULL — the column is nullable TEXT and +// we don't want to claim "approved by nobody". // // check is the caller's ClaimedRowCheck, run against the claimed row before // any provider call (issue #415); the token path passes nil. @@ -189,10 +189,11 @@ func OrphanExecutionError(execution *config.PurchaseExecution) error { // ApproveAndExecute) // // Adversarial review of PR #1500 found that requireDifferentApprover in -// internal/api was wired into only 2 of these 4 entry points, leaving +// internal/api was wired into only 2 of the 4 entry points then in use, leaving // execute_mode="direct" (HIGH: the requester always equals the creator on // this path, so this is unconditional self-authorization) able to bypass -// dual control entirely. This method closes that gap by running at the one place all four paths share. +// dual control entirely. This method closes that gap by running at the one place +// all of them share. // // Identity comparison has two tiers, checked in order of precision: // diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index c7ab7aa0c..06f7abce0 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -514,11 +514,12 @@ func TestManager_ApproveExecution_ReturnsRowOnExecutionFailure(t *testing.T) { // ApproveAndExecute is the ONE place every approve/execute entry point // funnels through: approvePurchaseViaSession, directExecutePurchase // (execute_mode="direct", issue #289), and ApproveExecution (both the -// email-token deep-link caller and the SQS async approve worker). The +// email-token deep-link caller, which passes an actor email and no +// transitionedBy). The // tests below exercise the manager-layer gate directly with the exact // (actor, transitionedBy) shapes each real caller passes, proving the two // bypasses adversarial review found on PR #1500 -- execute_mode="direct" -// (HIGH) and the SQS approve worker (MEDIUM) -- are now closed at the +// (HIGH) and the token-link approve call shape (MEDIUM) -- are now closed at the // shared choke point rather than only at the 2 HTTP handler call sites the // original PR wired requireDifferentApprover into. @@ -697,8 +698,8 @@ func TestManager_ApproveAndExecute_FourEyesOff_AllowsSelfApprove(t *testing.T) { sender.AssertExpectations(t) } -// TestManager_ApproveExecution_FourEyesOn_SQSActorEqualsCreator_Denied models -// the SQS async approve worker call shape (MEDIUM finding): ApproveExecution +// TestManager_ApproveExecution_FourEyesOn_TokenActorEqualsCreator_Denied models +// the token-link approve call shape (MEDIUM finding): ApproveExecution // always passes transitionedBy=nil, and the only identity signal is the // verified actor email. Pre-fix, ApproveExecution/ApproveAndExecute had no // awareness of 4-eyes mode at all, so a replayed or forwarded token+actor @@ -710,7 +711,7 @@ func TestManager_ApproveAndExecute_FourEyesOff_AllowsSelfApprove(t *testing.T) { // approval flow, so a creator whose stored email differs from the actor's // only in letter case is the same person and must still be denied. A // regression that swaps strings.EqualFold for == fails those rows. -func TestManager_ApproveExecution_FourEyesOn_SQSActorEqualsCreator_Denied(t *testing.T) { +func TestManager_ApproveExecution_FourEyesOn_TokenActorEqualsCreator_Denied(t *testing.T) { cases := []struct { name string creatorEmail string @@ -730,18 +731,18 @@ func TestManager_ApproveExecution_FourEyesOn_SQSActorEqualsCreator_Denied(t *tes creatorID := "user-creator" execution := &config.PurchaseExecution{ - ExecutionID: "exec-sqs-self", + ExecutionID: "exec-token-self", PlanID: "plan-fourEyes", Status: "pending", ApprovalToken: config.HashApprovalToken("valid-token"), CreatedByUserID: &creatorID, } - store.On("GetExecutionByID", ctx, "exec-sqs-self").Return(execution, nil) + store.On("GetExecutionByID", ctx, "exec-token-self").Return(execution, nil) store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) store.On("GetUserEmailByID", ctx, creatorID).Return(tc.creatorEmail, nil) - _, _, err := manager.ApproveExecution(ctx, "exec-sqs-self", "valid-token", tc.actorEmail, nil) + _, _, err := manager.ApproveExecution(ctx, "exec-token-self", "valid-token", tc.actorEmail, nil) require.ErrorIs(t, err, ErrFourEyesDenied) assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") store.AssertNotCalled(t, "TransitionExecutionStatusTokenPinned", @@ -752,7 +753,7 @@ func TestManager_ApproveExecution_FourEyesOn_SQSActorEqualsCreator_Denied(t *tes } // TestManager_ApproveExecution_FourEyesOn_DifferentActor_Allowed is the -// positive control for the SQS/token call shape: an actor that resolves to +// positive control for the token-link call shape: an actor that resolves to // a different email than the creator must still succeed. func TestManager_ApproveExecution_FourEyesOn_DifferentActor_Allowed(t *testing.T) { ctx := context.Background() @@ -760,14 +761,14 @@ func TestManager_ApproveExecution_FourEyesOn_DifferentActor_Allowed(t *testing.T creatorID := "user-creator" execution := &config.PurchaseExecution{ - ExecutionID: "exec-sqs-diff", + ExecutionID: "exec-token-diff", PlanID: "plan-fourEyes", Status: "pending", ApprovalToken: config.HashApprovalToken("valid-token"), CreatedByUserID: &creatorID, } updated := &config.PurchaseExecution{ - ExecutionID: "exec-sqs-diff", + ExecutionID: "exec-token-diff", PlanID: "plan-fourEyes", Status: "approved", ApprovalToken: config.HashApprovalToken("valid-token"), @@ -775,13 +776,13 @@ func TestManager_ApproveExecution_FourEyesOn_DifferentActor_Allowed(t *testing.T Recommendations: approvalTestRecs(), } - store.On("GetExecutionByID", ctx, "exec-sqs-diff").Return(execution, nil) + store.On("GetExecutionByID", ctx, "exec-token-diff").Return(execution, nil) store.On("GetGlobalConfig", mock.Anything).Return(fourEyesCfgOnForManager(), nil) store.On("GetUserEmailByID", ctx, creatorID).Return("creator@example.com", nil) - store.On("TransitionExecutionStatusTokenPinned", ctx, "exec-sqs-diff", approveFromStatuses, "running", (*string)(nil), "valid-token").Return(updated, nil) + store.On("TransitionExecutionStatusTokenPinned", ctx, "exec-token-diff", approveFromStatuses, "running", (*string)(nil), "valid-token").Return(updated, nil) stubExecuteChain(t, store, sender, "plan-fourEyes") - _, _, err := manager.ApproveExecution(ctx, "exec-sqs-diff", "valid-token", "approver@example.com", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-token-diff", "valid-token", "approver@example.com", nil) require.NoError(t, err) require.NotNil(t, updated.ApprovedBy) assert.Equal(t, "approver@example.com", *updated.ApprovedBy) @@ -804,7 +805,7 @@ func TestManager_ApproveExecution_FourEyesOn_UnresolvableCreator_Denied(t *testi creatorID := "user-creator-deleted" baseExecution := func() *config.PurchaseExecution { return &config.PurchaseExecution{ - ExecutionID: "exec-sqs-unresolvable", + ExecutionID: "exec-token-unresolvable", PlanID: "plan-fourEyes", Status: "pending", ApprovalToken: config.HashApprovalToken("valid-token"), @@ -816,13 +817,13 @@ func TestManager_ApproveExecution_FourEyesOn_UnresolvableCreator_Denied(t *testi ctx := context.Background() manager, store, _ := newApproveManager(t) - store.On("GetExecutionByID", ctx, "exec-sqs-unresolvable").Return(baseExecution(), nil) + store.On("GetExecutionByID", ctx, "exec-token-unresolvable").Return(baseExecution(), nil) store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) // Mirrors the real Postgres store's ("", nil) result for // pgx.ErrNoRows -- a deleted/nonexistent creator user row. store.On("GetUserEmailByID", ctx, creatorID).Return("", nil) - _, _, err := manager.ApproveExecution(ctx, "exec-sqs-unresolvable", "valid-token", "approver@example.com", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-token-unresolvable", "valid-token", "approver@example.com", nil) require.Error(t, err) assert.Contains(t, err.Error(), "creator's account could not be resolved") store.AssertNotCalled(t, "TransitionExecutionStatusTokenPinned", @@ -835,11 +836,11 @@ func TestManager_ApproveExecution_FourEyesOn_UnresolvableCreator_Denied(t *testi manager, store, _ := newApproveManager(t) lookupErr := errors.New("connection reset by peer") - store.On("GetExecutionByID", ctx, "exec-sqs-unresolvable").Return(baseExecution(), nil) + store.On("GetExecutionByID", ctx, "exec-token-unresolvable").Return(baseExecution(), nil) store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) store.On("GetUserEmailByID", ctx, creatorID).Return("", lookupErr) - _, _, err := manager.ApproveExecution(ctx, "exec-sqs-unresolvable", "valid-token", "approver@example.com", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-token-unresolvable", "valid-token", "approver@example.com", nil) require.Error(t, err) assert.Contains(t, err.Error(), "failed to resolve creator identity") store.AssertNotCalled(t, "TransitionExecutionStatusTokenPinned", @@ -1164,7 +1165,7 @@ func TestManager_ApproveExecution_NilExpiresAt_LegacyRow(t *testing.T) { // TestManager_ApproveExecution_NonAWSOrphanReturnsError is the regression // guard for issue #609 on the token-authenticated ApproveExecution path (used -// by the SQS approve worker and the legacy email-link flow). An execution +// by the token-link and legacy email-link flows). An execution // whose CloudAccountID is nil and whose provider is non-AWS must return a // descriptive error before the execute chain is entered, instead of surfacing // an opaque cloud-SDK credential error at runtime. diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 950f19bb0..eb88ae45e 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -221,7 +221,7 @@ func (e *multiAccountPartialError) Error() string { } // errAllAccountsFailed is returned by executeMultiAccount when no account -// committed any purchase — a flat failure that callers may redeliver/retry. +// committed any purchase — a flat failure that callers may retry. var errAllAccountsFailed = errors.New("multi-account execution: all accounts failed") // executeMultiAccount fans out executePurchase across all plan accounts in parallel. @@ -229,7 +229,7 @@ var errAllAccountsFailed = errors.New("multi-account execution: all accounts fai // // Returns nil when every account fully succeeded; a *multiAccountPartialError // when at least one account committed a purchase and at least one failed (so -// callers ACK rather than redeliver, issue #1014); and errAllAccountsFailed +// callers treat it as handled rather than retry, issue #1014); and errAllAccountsFailed // (wrapping the per-account errors) when no account committed anything. func (m *Manager) executeMultiAccount(ctx context.Context, baseExec *config.PurchaseExecution, plan *config.PurchasePlan, accounts []config.CloudAccount) error { // Checked before any account runs: every account would otherwise buy the diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 710f170d5..72f93b4bd 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -189,7 +189,7 @@ func (m *Manager) finalizeExecution(exec *config.PurchaseExecution, execErr erro // only proceeds when it wins that CAS. A lost CAS (row already claimed by a // concurrent worker, an overlapping cron tick, or the row // vanishing mid-flight) is benign: the function returns (claimed=false, nil) and -// the caller should ack/skip without re-running the purchase. A real DB error +// the caller should skip without re-running the purchase. A real DB error // during the claim returns (false, err). // // The claim is pinned to the recommendations snapshot the caller gated on: a row @@ -217,9 +217,9 @@ func (m *Manager) claimAndExecuteWithin(ctx context.Context, exec *config.Purcha updated, claimErr := m.config.TransitionExecutionStatusPinned(ctx, exec.ExecutionID, []string{"approved", "pending", "notified"}, "running", nil, gateSawEmpty) if claimErr != nil { if errors.Is(claimErr, config.ErrNotFound) || errors.Is(claimErr, config.ErrExecutionNotInExpectedStatus) { - // Benign CAS race-loss: another worker/redelivery already owns this - // row (or it was deleted), or its recommendations changed since the - // gate. Ack/skip without executing. + // Benign CAS race-loss: an overlapping sweep or approve already owns + // this row (or it was deleted), or its recommendations changed since + // the gate. Skip without executing. logging.Warnf("Skipping execution %s (CAS claim lost to concurrent worker or changed recommendations): %v", exec.ExecutionID, claimErr) return false, nil } @@ -250,11 +250,11 @@ func (m *Manager) claimedRowRefusal(ctx context.Context, exec *config.PurchaseEx return nil } -// isMultiAccountAckable reports whether a multi-account execErr should be ACKed -// (not redelivered/recounted as a flat failure) because at least one account +// isMultiAccountAckable reports whether a multi-account execErr should be +// treated as handled (not retried or counted as a flat failure) because at least one account // committed a real purchase (issue #1014). A nil error (full success) is also // ackable. errAllAccountsFailed is NOT ackable — nothing committed, so a -// redelivery is safe and useful. +// retry is safe and useful. func isMultiAccountAckable(execErr error) bool { if execErr == nil { return true diff --git a/internal/purchase/money_path_regression_test.go b/internal/purchase/money_path_regression_test.go index 0d6b46242..4fdfeaca1 100644 --- a/internal/purchase/money_path_regression_test.go +++ b/internal/purchase/money_path_regression_test.go @@ -334,16 +334,16 @@ func TestScheduledSweepOverlapDoesNotDoubleExecute(t *testing.T) { } } - // Both deliveries read the row as "pending" from the DB (pre-fix nothing CASes it to running - // before the cloud call, so an overlapping sweep re-reads a still-claimable row). Each GetExecutionByID returns a FRESH - // pending copy, faithfully modeling the real double-delivery scenario. + // Both sweeps read the row as claimable from the DB (pre-fix nothing CASes it to running + // before the cloud call, so an overlapping sweep re-reads a still-claimable row). Each + // GetPendingExecutions call returns a FRESH copy, modeling the real overlapping-tick scenario. mockStore.On("GetGlobalConfig", ctx).Return(autoPurchaseOnCfg(), nil).Once() mockStore.On("GetGlobalConfig", mock.Anything).Return(autoPurchaseOnCfg(), nil).Maybe() mockStore.On("GetStaleApprovedExecutions", ctx, mock.Anything).Return([]config.PurchaseExecution{}, nil) mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{*newPending()}, nil).Twice() // First claim wins: pending -> running, returns the running row. The second - // claim (redelivery) loses with ErrExecutionNotInExpectedStatus, because the + // claim (the overlapping tick) loses with ErrExecutionNotInExpectedStatus, because the // row already left the executable set. testify replays Once() expectations in // declaration order, so the first call gets the win and the second the loss. running := newPending() @@ -385,7 +385,7 @@ func TestScheduledSweepOverlapDoesNotDoubleExecute(t *testing.T) { mu.Lock() defer mu.Unlock() - assert.Equal(t, 1, purchaseCalls, "the cloud purchase must run EXACTLY once across both deliveries (issue #1013)") + assert.Equal(t, 1, purchaseCalls, "the cloud purchase must run EXACTLY once across both sweeps (issue #1013)") } // TestMultiAccountPartialSuccessIsAcked is the issue #1014 regression guard: a @@ -473,7 +473,7 @@ func TestMultiAccountPartialSuccessIsAcked(t *testing.T) { result, err := manager.ProcessScheduledPurchases(ctx) require.NoError(t, err) assert.Equal(t, 1, result.Executed, - "a multi-account run with >=1 committed account must be ACKed (counted Executed), not a flat failure (issue #1014)") + "a multi-account run with >=1 committed account must be counted Executed, not a flat failure (issue #1014)") assert.Equal(t, 0, result.Failed) require.NotNil(t, savedRoot, "the root row must be saved with its aggregate status (H2)") diff --git a/known-issues.md b/known-issues.md index 12db50c2c..e16a996d1 100644 --- a/known-issues.md +++ b/known-issues.md @@ -7,8 +7,8 @@ action. Resolved items are moved to the Resolved section at the bottom. ### `claimAndExecuteWithin` still claims from `approved` (issue #399 follow-up) -The claim funnel's from-set is `approved, pending, notified`. With the SQS -execute path gone, no executor reaches it with an `approved` row (the +The claim funnel's from-set is `approved, pending, notified`. With the +execute-by-ID path gone, no executor reaches it with an `approved` row (the approve paths claim `pending, notified` and stranded `approved` rows go through the re-drive), so the `approved` entry is unreachable in production. Left in place by issue #399 to keep that change a pure removal; dropping it From 7cfc88c0414fc032c33cff320f8ca9eea64505d8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 20:12:58 +0100 Subject: [PATCH 14/14] test(purchase): run main's new Redshift gate tests through claimAndExecuteWithin Main added three integration tests that call claimAndExecute, which this branch removes. Run them through claimAndExecuteWithin on notified rows, like the rest of the file. Refs #399 Co-Authored-By: Claude Opus 5.5 --- internal/purchase/redshift_gate_integration_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/purchase/redshift_gate_integration_test.go b/internal/purchase/redshift_gate_integration_test.go index e4dda124b..19e1d1ab6 100644 --- a/internal/purchase/redshift_gate_integration_test.go +++ b/internal/purchase/redshift_gate_integration_test.go @@ -196,9 +196,9 @@ func TestRedshiftDispatchGate_RecentCompletedBuyRefusedBeforeSubmission(t *testi ctx := context.Background() f := newRedshiftGateFixture(ctx, t) f.finishedRow(ctx, t, uuid.New().String()+":"+f.accounts[0].ID) - refused := f.redshiftRow(ctx, t, "approved") + refused := f.redshiftRow(ctx, t, "notified") - claimed, _ := f.mgr.claimAndExecute(ctx, rowStatusCopy(refused)) + claimed, _ := f.mgr.claimAndExecuteWithin(ctx, rowStatusCopy(refused), 0) require.True(t, claimed) assert.Zero(t, f.bought.Load(), "a refused lineage never reaches the provider") @@ -215,12 +215,12 @@ func TestRedshiftDispatchGate_SecondLineageAfterWindowDispatches(t *testing.T) { ctx := context.Background() f := newRedshiftGateFixture(ctx, t) f.finishedRow(ctx, t, uuid.New().String()+":"+f.accounts[0].ID) - later := f.redshiftRow(ctx, t, "approved") + later := f.redshiftRow(ctx, t, "notified") _, err := f.pool.Exec(ctx, `UPDATE purchase_executions SET first_dispatch_at = now() + $2 * interval '1 second' WHERE execution_id = $1`, later.ExecutionID, (redshiftAdoptionWindow + time.Minute).Seconds()) require.NoError(t, err) - claimed, _ := f.mgr.claimAndExecute(ctx, rowStatusCopy(later)) + claimed, _ := f.mgr.claimAndExecuteWithin(ctx, rowStatusCopy(later), 0) require.True(t, claimed) assert.Equal(t, int32(1), f.bought.Load()) @@ -234,9 +234,9 @@ func TestRedshiftDispatchGate_SameFamilyCompletedRowNotRefused(t *testing.T) { f := newRedshiftGateFixture(ctx, t) family := uuid.New().String() f.finishedRow(ctx, t, family+":"+f.accounts[0].ID) - successor := f.redshiftRowKeyed(ctx, t, "approved", family, true) + successor := f.redshiftRowKeyed(ctx, t, "notified", family, true) - claimed, _ := f.mgr.claimAndExecute(ctx, rowStatusCopy(successor)) + claimed, _ := f.mgr.claimAndExecuteWithin(ctx, rowStatusCopy(successor), 0) require.True(t, claimed) assert.NotEqual(t, redshiftRecentBuyRefusal, SanitizeStoredError(rowStatus(ctx, t, f.rampStepFixture, successor.ExecutionID).Error))