fix(api/plans): differentiate ErrNotFound->404 + add structured slog on createPlan errors (closes #944)

internal/api/handler_plans.go

@@ -4,7 +4,9 @@ package api
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
+	"net/http"
 	"time"
 
 	"github.com/LeanerCloud/CUDly/internal/config"
@@ -62,6 +64,18 @@ func calculateNextExecutionDate(plan *config.PurchasePlan, now time.Time) *time.
 	return &nextDate
 }
 
+// mapCreatePlanStorageError converts a storage-layer error from createPlan into
+// the appropriate HTTP ClientError. If err is ErrNotFound it returns a 404
+// with notFoundMsg; otherwise it logs the supplied format string at ERROR level
+// and returns a 500 with genericMsg.
+func mapCreatePlanStorageError(err error, notFoundMsg, genericMsg, logFmt string, logArgs ...any) error {
+	if errors.Is(err, config.ErrNotFound) {
+		return NewClientError(http.StatusNotFound, notFoundMsg)
+	}
+	logging.Errorf(logFmt, logArgs...)
+	return NewClientError(http.StatusInternalServerError, genericMsg)
+}
+
 func (h *Handler) createPlan(ctx context.Context, httpReq *events.LambdaFunctionURLRequest) (any, error) {
 	// Require create:plans permission
 	if _, err := h.requirePermission(ctx, httpReq, "create", "plans"); err != nil {
@@ -91,34 +105,34 @@ func (h *Handler) createPlan(ctx context.Context, httpReq *events.LambdaFunction
 	}
 
 	if err := h.config.CreatePurchasePlan(ctx, plan); err != nil {
-		return nil, err
+		return nil, mapCreatePlanStorageError(err,
+			"plan not found", "failed to create plan",
+			"createPlan: CreatePurchasePlan failed (provider=%s service=%s accounts=%d): %v",
+			req.Provider, req.Service, len(req.TargetAccounts), err)
 	}
 
 	// Provider-match validation + plan_accounts insert. SetPlanAccounts is
 	// transactional internally, but the plan-row insert above is not part of
 	// that tx. If either step here fails we roll the plan row back so the
 	// invariant "every purchase_plans row has at least one plan_accounts
-	// row" holds end-to-end — otherwise a validation failure would leave a
-	// fresh universal plan behind, which is exactly the bug class we're
-	// eliminating.
-	//
-	// rollbackPlan undoes the partial CreatePurchasePlan insert. If the
-	// rollback delete itself errors (DB blip, row already gone, etc.), log
-	// at WARN with the plan ID so an operator can clean up manually — we
-	// still surface the original cause to the caller so the user-facing
-	// error is unchanged.
+	// row" holds end-to-end.
 	rollbackPlan := func() {
 		if delErr := h.config.DeletePurchasePlan(ctx, plan.ID); delErr != nil {
 			logging.Warnf("createPlan rollback: failed to delete partial plan %s: %v (manual cleanup may be required)", plan.ID, delErr)
 		}
 	}
+
 	if err := h.validatePlanAccountProviders(ctx, plan.ID, req.TargetAccounts); err != nil {
 		rollbackPlan()
 		return nil, err
 	}
+
 	if err := h.config.SetPlanAccounts(ctx, plan.ID, req.TargetAccounts); err != nil {
 		rollbackPlan()
-		return nil, fmt.Errorf("accounts: %w", err)
+		return nil, mapCreatePlanStorageError(err,
+			"account not found", "failed to assign accounts to plan",
+			"createPlan: SetPlanAccounts failed (plan=%s accounts=%d): %v",
+			plan.ID, len(req.TargetAccounts), err)
 	}
 
 	return plan, nil

internal/api/handler_plans_test.go

@@ -196,12 +196,10 @@ func TestHandler_createPlan_RejectsEmptyTargetAccounts(t *testing.T) {
 
 // TestHandler_createPlan_RollbackDeleteFailureSurfacesOriginalError verifies
 // that when SetPlanAccounts fails after CreatePurchasePlan succeeds, AND the
-// rollback DeletePurchasePlan also fails, the caller still receives the
-// original SetPlanAccounts error (wrapped as "accounts: …") rather than the
-// rollback error. The rollback failure is logged at WARN so an operator can
-// clean up manually — see handler_plans.go createPlan rollbackPlan closure.
-// Regression guard for CR #743 finding F1: rollback errors must not be
-// silently discarded.
+// rollback DeletePurchasePlan also fails, the caller still receives a 500
+// ClientError (not the raw DB error) and the rollback error does not leak.
+// The rollback failure is logged at WARN; the SetPlanAccounts error is logged
+// at ERROR -- see handler_plans.go createPlan rollbackPlan closure.
 func TestHandler_createPlan_RollbackDeleteFailureSurfacesOriginalError(t *testing.T) {
 	ctx := context.Background()
 	mockStore := new(MockConfigStore)
@@ -238,12 +236,13 @@ func TestHandler_createPlan_RollbackDeleteFailureSurfacesOriginalError(t *testin
 	result, err := handler.createPlan(ctx, req)
 	assert.Nil(t, result)
 	require.Error(t, err)
-	// Original error from SetPlanAccounts is what reaches the caller —
-	// wrapped as "accounts: …" per the handler. The rollback error is logged
-	// (not returned), so it must NOT appear in the user-facing error chain.
-	assert.Contains(t, err.Error(), "accounts:")
-	assert.True(t, errors.Is(err, setAccountsErr), "expected wrapped SetPlanAccounts error, got %v", err)
-	assert.NotContains(t, err.Error(), "rollback delete boom", "rollback error must not leak into user-facing error")
+	// Handler returns a 500 ClientError with a safe generic message; raw DB
+	// errors are logged, not returned. Rollback error also must not leak.
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected ClientError, got %T: %v", err, err)
+	assert.Equal(t, 500, ce.code)
+	assert.NotContains(t, ce.Error(), "setplanaccounts boom", "DB error must not leak to caller")
+	assert.NotContains(t, ce.Error(), "rollback delete boom", "rollback error must not leak to caller")
 	mockStore.AssertCalled(t, "DeletePurchasePlan", ctx, mock.AnythingOfType("string"))
 }
 
@@ -1035,3 +1034,89 @@ func TestHandler_patchPlan_UpdateError(t *testing.T) {
 	assert.Error(t, err)
 	assert.Nil(t, result)
 }
+
+// TestHandler_createPlan_UnknownAccountReturns404 is the regression guard for
+// the 500/404 inconsistency: createPlan with a target_accounts entry whose
+// UUID is not present in the store must return 404, not 500.
+func TestHandler_createPlan_UnknownAccountReturns404(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockConfigStore)
+	mockAuth := new(MockAuthService)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+
+	adminSession := &Session{
+		UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa",
+		Email:  "admin@example.com",
+	}
+	unknownAccountID := "cccccccc-cccc-cccc-cccc-cccccccccccc"
+
+	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
+	mockAuth.grantAdmin()
+	mockStore.On("CreatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil)
+	// Rollback is triggered when validatePlanAccountProviders returns an error.
+	mockStore.On("DeletePurchasePlan", ctx, mock.AnythingOfType("string")).Return(nil)
+	// GetPurchasePlan is called by getPlanForAccountProviderValidation inside
+	// validatePlanAccountProviders. Return a plan with services so
+	// DerivePlanProviders produces a non-empty expected set and the
+	// provider-match check actually runs (empty services => check skipped).
+	mockStore.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) {
+		return &config.PurchasePlan{
+			Services: map[string]config.ServiceConfig{
+				"aws/rds": {Provider: "aws", Service: "rds"},
+			},
+		}, nil
+	}
+	// GetCloudAccount returns nil (not found) to simulate the missing account.
+	mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) {
+		return nil, nil // nil account, nil error => "not found" branch
+	}
+
+	handler := &Handler{config: mockStore, auth: mockAuth}
+	body := `{"name": "P", "provider": "aws", "service": "rds", "target_accounts": ["` + unknownAccountID + `"]}`
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"Authorization": "Bearer admin-token"},
+		Body:    body,
+	}
+	result, err := handler.createPlan(ctx, req)
+	assert.Nil(t, result)
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected ClientError (not opaque 500), got %T: %v", err, err)
+	assert.Equal(t, 404, ce.code, "unknown account must return 404, not 500")
+}
+
+// TestHandler_createPlan_DBErrorOnCreateReturns500WithLog verifies that a DB
+// failure on CreatePurchasePlan returns a well-formed 500 ClientError instead
+// of a raw unwrapped error (which would look the same but is less explicit).
+func TestHandler_createPlan_DBErrorOnCreateReturns500WithLog(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockConfigStore)
+	mockAuth := new(MockAuthService)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+
+	adminSession := &Session{
+		UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa",
+		Email:  "admin@example.com",
+	}
+	targetAccountID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"
+
+	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
+	mockAuth.grantAdmin()
+	dbErr := errors.New("connection refused")
+	mockStore.On("CreatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(dbErr)
+
+	handler := &Handler{config: mockStore, auth: mockAuth}
+	body := `{"name": "P", "provider": "aws", "service": "rds", "target_accounts": ["` + targetAccountID + `"]}`
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"Authorization": "Bearer admin-token"},
+		Body:    body,
+	}
+	result, err := handler.createPlan(ctx, req)
+	assert.Nil(t, result)
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "DB error must be wrapped as ClientError, got %T: %v", err, err)
+	assert.Equal(t, 500, ce.code)
+	// Raw DB error must NOT be exposed to the caller.
+	assert.NotContains(t, ce.Error(), "connection refused", "internal DB error must not leak to caller")
+}

internal/api/handler_test.go

@@ -575,6 +575,8 @@ func TestHandler_HandleRequest_CreatePlan(t *testing.T) {
 	mockStore := new(MockConfigStore)
 	mockAuth := new(MockAuthService)
 
+	targetAccountID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"
+
 	adminSession := &Session{UserID: "admin-id", Email: "admin@example.com"}
 	mockAuth.On("ValidateSession", ctx, "test-token").Return(adminSession, nil)
 	mockAuth.grantAdmin()
@@ -601,7 +603,7 @@ func TestHandler_HandleRequest_CreatePlan(t *testing.T) {
 		// target_accounts is required (universal-plans fix). Provider must
 		// also be set so DerivePlanProviders returns non-empty; otherwise
 		// the validation skip-branch would mask the contract.
-		Body: `{"name": "New Plan", "provider": "aws", "service": "rds", "target_accounts": ["11111111-1111-1111-1111-111111111111"]}`,
+		Body: `{"name": "New Plan", "provider": "aws", "service": "rds", "target_accounts": ["` + targetAccountID + `"]}`,
 		RequestContext: events.LambdaFunctionURLRequestContext{
 			HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{
 				Method: "POST",

internal/api/types.go

@@ -661,7 +661,7 @@ type PlanRequest struct {
 	CustomIntervalDays int    `json:"custom_interval_days,omitempty"`
 
 	// TargetAccounts is the list of cloud_account UUIDs the plan will purchase
-	// for. Required (non-empty) on POST /plans — a plan with no rows in
+	// for. Required (non-empty) on POST /plans -- a plan with no rows in
 	// plan_accounts is a "universal plan", which the design no longer allows:
 	// every plan must be tied to at least one explicit account. The handler
 	// inserts the plan_accounts rows immediately after CreatePurchasePlan so