From f2ce4568f44594bf6f73504a77d56159b6f93088 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 3 Jul 2026 15:08:19 +0200 Subject: [PATCH 1/5] fix(deps): bump github.com/jackc/pgx/v5 to v5.9.2 for GO-2026-5004 Fixes govulncheck finding GO-2026-5004 (SQL injection via dollar-quoted string constants in the simple protocol) reported by CI on main since 2026-06-11. --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index de601d7dc..f308e0402 100644 --- a/go.mod +++ b/go.mod @@ -109,7 +109,7 @@ require ( github.com/go-jose/go-jose/v4 v4.1.4 github.com/golang-migrate/migrate/v4 v4.19.1 github.com/google/uuid v1.6.0 - github.com/jackc/pgx/v5 v5.8.0 + github.com/jackc/pgx/v5 v5.9.2 github.com/pashagolub/pgxmock/v4 v4.9.0 github.com/testcontainers/testcontainers-go v0.42.0 github.com/testcontainers/testcontainers-go/modules/postgres v0.42.0 diff --git a/go.sum b/go.sum index a726f9e1a..a25417d6b 100644 --- a/go.sum +++ b/go.sum @@ -250,8 +250,8 @@ github.com/jackc/pgpassfile v1.0.0 h1:/6Hmqy13Ss2zCq62VdNG8tM1wchn8zjSGOBJ6icpsI github.com/jackc/pgpassfile v1.0.0/go.mod h1:CEx0iS5ambNFdcRtxPj5JhEz+xB6uRky5eyVu/W2HEg= github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 h1:iCEnooe7UlwOQYpKFhBabPMi4aNAfoODPEFNiAnClxo= github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761/go.mod h1:5TJZWKEWniPve33vlWYSoGYefn3gLQRzjfDlhSJ9ZKM= -github.com/jackc/pgx/v5 v5.8.0 h1:TYPDoleBBme0xGSAX3/+NujXXtpZn9HBONkQC7IEZSo= -github.com/jackc/pgx/v5 v5.8.0/go.mod h1:QVeDInX2m9VyzvNeiCJVjCkNFqzsNb43204HshNSZKw= +github.com/jackc/pgx/v5 v5.9.2 h1:3ZhOzMWnR4yJ+RW1XImIPsD1aNSz4T4fyP7zlQb56hw= +github.com/jackc/pgx/v5 v5.9.2/go.mod h1:mal1tBGAFfLHvZzaYh77YS/eC6IX9OWbRV1QIIM0Jn4= github.com/jackc/puddle/v2 v2.2.2 h1:PR8nw+E/1w0GLuRFSmiioY6UooMp6KJv0/61nB7icHo= github.com/jackc/puddle/v2 v2.2.2/go.mod h1:vriiEXHvEE654aYKXXjOvZM39qJ0q+azkZFrfEOc3H4= github.com/keybase/go-keychain v0.0.1 h1:way+bWYa6lDppZoZcgMbYsvC7GxljxrskdNInRtuthU= From c7cd25af95cb66987f9687ca4871c91684b7e67a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 3 Jul 2026 15:08:30 +0200 Subject: [PATCH 2/5] fix(lint): address all errcheck findings across cmd/ and internal/ golangci-lint v2.10.1 errcheck with check-blank:true and check-type-assertions:true flagged unchecked error returns and single-value type assertions in 29 files. Changes by category: - Defer rollback errors: log instead of blank-assign (cleanup-lambda, rekey, commitmentopts/store_postgres) - Interactive CLI ReadString errors: return error to caller (configure_azure, configure_gcp) - buildResponse: drop unused error return; callers use single-return form; update two test callers accordingly (handler, handler_test, handler_coverage_test) - Intentionally best-effort operations: capture error and log at warn/ debug (handler.go resolveSourceIdentity STS, dashboard resolveAWS*, recommendations_refresh ClearCollectionStarted x2, registrations requireAdmin, ri_utilization_cache singleflight, service_mfa UpdateUser, scheduler errgroup.Wait x2 and GetRecommendations, execution SavePurchaseExecution, credentials resolver LoadRaw, server/http w.Write) - singleflight.Do: use two-value if-init form so error is captured (service_apikeys, ri_utilization_cache) - Type assertions: two-value form with panic on mismatch in mocks (stores, secretsmanager, ses, sns - 57 sites) and test_helpers (auth - 10 sites) - Type assertion: validation.go gcp payload type field - Connection pool: ok-check on sync.Map type assertion (connection.go) - DevKey: panic on invalid compile-time hex constant (cipher.go) - migrate.go: capture version check error before log - testhelpers: log container Terminate error in cleanup path - handler_accounts: add logging import, fail-closed on HasCredential error --- cmd/cleanup-lambda/main.go | 4 +- cmd/configure_azure.go | 53 ++- cmd/configure_gcp.go | 40 ++- cmd/rekey/main.go | 4 +- internal/api/handler.go | 44 +-- internal/api/handler_accounts.go | 8 +- internal/api/handler_coverage_test.go | 11 +- internal/api/handler_dashboard.go | 10 +- .../api/handler_recommendations_refresh.go | 14 +- internal/api/handler_registrations.go | 5 +- internal/api/handler_test.go | 7 +- internal/api/ri_utilization_cache.go | 7 +- internal/api/validation.go | 13 +- internal/auth/service_apikeys.go | 6 +- internal/auth/service_mfa.go | 5 +- internal/auth/test_helpers.go | 61 +++- internal/commitmentopts/store_postgres.go | 7 +- internal/credentials/cipher.go | 6 +- internal/credentials/resolver.go | 7 +- internal/database/connection.go | 6 +- .../database/postgres/migrations/migrate.go | 4 +- .../database/postgres/testhelpers/postgres.go | 5 +- internal/mocks/secretsmanager.go | 19 +- internal/mocks/ses.go | 7 +- internal/mocks/sns.go | 7 +- internal/mocks/stores.go | 313 +++++++++++++++--- internal/purchase/execution.go | 11 +- internal/scheduler/scheduler.go | 16 +- internal/server/http.go | 4 +- 29 files changed, 551 insertions(+), 153 deletions(-) diff --git a/cmd/cleanup-lambda/main.go b/cmd/cleanup-lambda/main.go index ed3cc8309..6b7f11377 100644 --- a/cmd/cleanup-lambda/main.go +++ b/cmd/cleanup-lambda/main.go @@ -75,7 +75,9 @@ func deleteExpired(ctx context.Context, db *database.Connection, now time.Time, } defer func() { if err != nil { - _ = tx.Rollback(ctx) + if rErr := tx.Rollback(ctx); rErr != nil { + log.Printf("rollback failed: %v", rErr) + } } }() diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 2cafee463..8589b2a76 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -277,7 +277,10 @@ func runAzureSetupCommands(reader *bufio.Reader) error { fmt.Println() fmt.Print("Enter your Subscription ID from above: ") - subscriptionID, _ := reader.ReadString('\n') + subscriptionID, err := reader.ReadString('\n') + if err != nil { + return fmt.Errorf("failed to read subscription ID: %w", err) + } subscriptionID = strings.TrimSpace(subscriptionID) if subscriptionID == "" { @@ -289,19 +292,37 @@ func runAzureSetupCommands(reader *bufio.Reader) error { return err } + if err := createAzureServicePrincipal(reader, subscriptionID); err != nil { + return err + } + + fmt.Println() + fmt.Println("IMPORTANT: Copy the output above! You'll need:") + fmt.Println(" - appId -> Client ID") + fmt.Println(" - password -> Client Secret") + fmt.Println(" - tenant -> Tenant ID") + fmt.Printf(" - Subscription ID: %s\n", subscriptionID) + fmt.Println() + + return nil +} + +// createAzureServicePrincipal runs Step 3 of Azure setup: create service principal. +func createAzureServicePrincipal(reader *bufio.Reader, subscriptionID string) error { fmt.Println() fmt.Println("Step 3: Create Service Principal") fmt.Println("---------------------------------") fmt.Println("This creates an Azure Service Principal with Reservation Administrator role.") fmt.Println() - // Build the create SP command - run directly without shell to avoid injection - // Using exec.Command directly with proper arguments fmt.Printf("Command: az ad sp create-for-rbac --name CUDly --role \"Reservations Administrator\" --scopes /subscriptions/%s\n", subscriptionID) fmt.Println() fmt.Printf("[R]un, [S]kip? ") - choice, _ := reader.ReadString('\n') + choice, err := reader.ReadString('\n') + if err != nil { + return fmt.Errorf("failed to read choice: %w", err) + } choice = strings.ToLower(strings.TrimSpace(choice)) if choice == "r" || choice == "run" || choice == "" { @@ -317,7 +338,10 @@ func runAzureSetupCommands(reader *bufio.Reader) error { if err := cmd.Run(); err != nil { fmt.Printf("Command failed: %v\n", err) fmt.Print("Continue anyway? [y/N]: ") - response, _ := reader.ReadString('\n') + response, readErr := reader.ReadString('\n') + if readErr != nil { + return fmt.Errorf("failed to read response: %w", readErr) + } if strings.ToLower(strings.TrimSpace(response)) != "y" { return fmt.Errorf("failed to create service principal: %w", err) } @@ -326,15 +350,6 @@ func runAzureSetupCommands(reader *bufio.Reader) error { } else { fmt.Println("Skipping Create Service Principal") } - - fmt.Println() - fmt.Println("IMPORTANT: Copy the output above! You'll need:") - fmt.Println(" - appId -> Client ID") - fmt.Println(" - password -> Client Secret") - fmt.Println(" - tenant -> Tenant ID") - fmt.Printf(" - Subscription ID: %s\n", subscriptionID) - fmt.Println() - return nil } @@ -345,7 +360,10 @@ func promptAndRunExplicitCommand(reader *bufio.Reader, name, displayCmd string, fmt.Println() fmt.Printf("[R]un, [S]kip? ") - choice, _ := reader.ReadString('\n') + choice, err := reader.ReadString('\n') + if err != nil { + return fmt.Errorf("failed to read choice: %w", err) + } choice = strings.ToLower(strings.TrimSpace(choice)) switch choice { @@ -378,7 +396,10 @@ func executeExplicitCommand(displayCmd string, program string, args ...string) e fmt.Printf("Command failed: %v\n", err) fmt.Print("Continue anyway? [y/N]: ") reader := bufio.NewReader(os.Stdin) - response, _ := reader.ReadString('\n') + response, readErr := reader.ReadString('\n') + if readErr != nil { + return fmt.Errorf("failed to read response: %w", readErr) + } if strings.ToLower(strings.TrimSpace(response)) != "y" { return fmt.Errorf("command failed: %w", err) } diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index fbe687016..e756940ef 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -177,7 +177,11 @@ func getGCPCredentialsFilePath(reader *bufio.Reader) (string, error) { if credsFile == "" { fmt.Print("Path to GCP service account JSON key file: ") - credsFile, _ = reader.ReadString('\n') + var readErr error + credsFile, readErr = reader.ReadString('\n') + if readErr != nil { + return "", fmt.Errorf("failed to read credentials file path: %w", readErr) + } credsFile = strings.TrimSpace(credsFile) } @@ -273,12 +277,9 @@ func runGCPSetupCommands(reader *bufio.Reader) (string, error) { } fmt.Println() - fmt.Print("Enter your Project ID from above: ") - projectID, _ := reader.ReadString('\n') - projectID = strings.TrimSpace(projectID) - - if projectID == "" { - return "", fmt.Errorf("project ID is required") + projectID, err := readRequiredInputLine(reader, "Enter your Project ID from above: ", "project ID") + if err != nil { + return "", err } // Validate project ID to prevent command injection @@ -358,6 +359,21 @@ func runGCPSetupCommands(reader *bufio.Reader) (string, error) { return keyFile, nil } +// readRequiredInputLine prints prompt, reads a line, trims whitespace, and +// returns an error if the result is empty. +func readRequiredInputLine(reader *bufio.Reader, prompt, fieldName string) (string, error) { + fmt.Print(prompt) + value, err := reader.ReadString('\n') + if err != nil { + return "", fmt.Errorf("failed to read %s: %w", fieldName, err) + } + value = strings.TrimSpace(value) + if value == "" { + return "", fmt.Errorf("%s is required", fieldName) + } + return value, nil +} + // promptAndRunGCPCommand shows a command and asks to run or skip. // Takes explicit program and args to avoid command injection via string splitting. func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd string, program string, args ...string) error { @@ -365,7 +381,10 @@ func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd string, progr fmt.Println() fmt.Printf("[R]un, [S]kip? ") - choice, _ := reader.ReadString('\n') + choice, err := reader.ReadString('\n') + if err != nil { + return fmt.Errorf("failed to read choice: %w", err) + } choice = strings.ToLower(strings.TrimSpace(choice)) switch choice { @@ -398,7 +417,10 @@ func executeGCPCommand(displayCmd string, program string, args ...string) error fmt.Printf("Command failed: %v\n", err) fmt.Print("Continue anyway? [y/N]: ") reader := bufio.NewReader(os.Stdin) - response, _ := reader.ReadString('\n') + response, readErr := reader.ReadString('\n') + if readErr != nil { + return fmt.Errorf("failed to read response: %w", readErr) + } if strings.ToLower(strings.TrimSpace(response)) != "y" { return fmt.Errorf("command failed: %w", err) } diff --git a/cmd/rekey/main.go b/cmd/rekey/main.go index ed61eefea..e4084fc57 100644 --- a/cmd/rekey/main.go +++ b/cmd/rekey/main.go @@ -181,7 +181,9 @@ func rekeyOne(ctx context.Context, db *database.Connection, id, blob string, zer return outcomeErrored } if _, err := tx.Exec(ctx, `UPDATE account_credentials SET encrypted_blob = $1 WHERE id = $2`, newBlob, id); err != nil { - _ = tx.Rollback(ctx) + if rErr := tx.Rollback(ctx); rErr != nil { + log.Printf("rekey: rollback id=%s: %v", id, rErr) + } log.Printf("rekey: update id=%s: %v", id, err) return outcomeErrored } diff --git a/internal/api/handler.go b/internal/api/handler.go index 28bd61aec..bfaaefcda 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -286,15 +286,14 @@ func setSecurityHeaders(headers map[string]string) map[string]string { // HandleRequest processes a Lambda Function URL request func (h *Handler) HandleRequest(ctx context.Context, req *events.LambdaFunctionURLRequest) (*events.LambdaFunctionURLResponse, error) { if req == nil { - resp, _ := h.buildResponse(400, h.buildResponseHeaders(), map[string]string{"error": "nil request"}, nil) - return resp, nil + return h.buildResponse(400, h.buildResponseHeaders(), map[string]string{"error": "nil request"}, nil), nil } corsHeaders := h.buildResponseHeaders() // Handle preflight method := req.RequestContext.HTTP.Method if method == "OPTIONS" { - return h.buildResponse(200, corsHeaders, nil, nil) + return h.buildResponse(200, corsHeaders, nil, nil), nil } path := req.RequestContext.HTTP.Path @@ -332,14 +331,12 @@ func (h *Handler) validateRequest(ctx context.Context, req *events.LambdaFunctio // Validate request body size if err := validateRequestBodySize(req.Body); err != nil { logging.Warnf("Request body size exceeded: %d bytes", len(req.Body)) - resp, _ := h.buildResponse(413, corsHeaders, map[string]string{"error": "Request body too large"}, nil) - return resp + return h.buildResponse(413, corsHeaders, map[string]string{"error": "Request body too large"}, nil) } // Validate Content-Type if err := validateContentType(req); err != nil { - resp, _ := h.buildResponse(415, corsHeaders, map[string]string{"error": err.Error()}, nil) - return resp + return h.buildResponse(415, corsHeaders, map[string]string{"error": err.Error()}, nil) } // Validate authentication and CSRF @@ -357,15 +354,13 @@ func (h *Handler) validateSecurity(ctx context.Context, req *events.LambdaFuncti } if !h.authenticate(ctx, req) { - resp, _ := h.buildResponse(401, corsHeaders, map[string]string{"error": "Unauthorized"}, nil) - return resp + return h.buildResponse(401, corsHeaders, map[string]string{"error": "Unauthorized"}, nil) } if h.requiresCSRFValidation(method, path, req) { if err := h.validateCSRF(ctx, req); err != nil { logging.Warnf("CSRF validation failed: %v", err) - resp, _ := h.buildResponse(403, corsHeaders, map[string]string{"error": "CSRF validation failed"}, nil) - return resp + return h.buildResponse(403, corsHeaders, map[string]string{"error": "CSRF validation failed"}, nil) } } @@ -381,7 +376,7 @@ func (h *Handler) executeRequest(ctx context.Context, method, path string, req * statusCode, response = h.handleRequestError(err) } - return h.buildResponse(statusCode, corsHeaders, response, nil) + return h.buildResponse(statusCode, corsHeaders, response, nil), nil } // handleRequestError converts an error to status code and response @@ -428,14 +423,16 @@ type rawResponse struct { csp string } -// buildResponse creates a Lambda Function URL response -func (h *Handler) buildResponse(statusCode int, headers map[string]string, body any, err error) (*events.LambdaFunctionURLResponse, error) { +// buildResponse creates a Lambda Function URL response. It never returns an +// error: all failure modes (marshal failure, non-nil err arg) are converted +// into a 500 response body so callers can use the single-return form. +func (h *Handler) buildResponse(statusCode int, headers map[string]string, body any, err error) *events.LambdaFunctionURLResponse { if err != nil { return &events.LambdaFunctionURLResponse{ StatusCode: 500, Headers: headers, Body: `{"error": "internal server error"}`, - }, nil + } } // Handle raw (non-JSON) responses @@ -448,7 +445,7 @@ func (h *Handler) buildResponse(statusCode int, headers map[string]string, body StatusCode: statusCode, Headers: headers, Body: raw.body, - }, nil + } } var bodyBytes []byte @@ -461,7 +458,7 @@ func (h *Handler) buildResponse(statusCode int, headers map[string]string, body StatusCode: 500, Headers: headers, Body: `{"error": "internal server error"}`, - }, nil + } } } else { // Nil-body success paths (e.g. DELETE /accounts/:id returning @@ -476,7 +473,7 @@ func (h *Handler) buildResponse(statusCode int, headers map[string]string, body StatusCode: statusCode, Headers: headers, Body: string(bodyBytes), - }, nil + } } // loadAPIKey retrieves the API key from Secrets Manager. @@ -554,12 +551,15 @@ func (h *Handler) resolveSourceIdentity(ctx context.Context) *sourceIdentity { // resolveSourceIdentity is best-effort and is consumed by // populateSourceAccountID, which fails loud on an empty // AccountID for the AWS-source case. STS errors are already - // logged WARN inside resolveAWSCallerIdentity, so we drop - // the error here explicitly — the consumer's empty-string - // check is the security gate for federation rendering. + // logged WARN inside resolveAWSCallerIdentity; the consumer's + // empty-string check is the security gate for federation rendering. // (Reshape uses resolveAWSAccountID directly which DOES // propagate the error for fail-closed multi-tenant safety.) - id.AccountID, id.Partition, _ = h.resolveAWSCallerIdentity(ctx) + var stsErr error + id.AccountID, id.Partition, stsErr = h.resolveAWSCallerIdentity(ctx) + if stsErr != nil { + logging.Debugf("resolveSourceIdentity: best-effort STS failed (already logged): %v", stsErr) + } case "azure": id.ClientID = os.Getenv("AZURE_CLIENT_ID") id.SubscriptionID = os.Getenv("AZURE_SUBSCRIPTION_ID") diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index 095e81cdb..4161357b1 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -17,6 +17,7 @@ import ( "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/internal/credentials" "github.com/LeanerCloud/CUDly/internal/oidc" + "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/aws/aws-lambda-go/events" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/sts" @@ -817,7 +818,12 @@ func (h *Handler) canUseGCPFederated(ctx context.Context, acct *config.CloudAcco // If a legacy stored WIF JSON is present, defer to the presence // check so legacy accounts keep reporting the same shape. if h.credStore != nil { - if has, _ := h.credStore.HasCredential(ctx, acct.ID, credentials.CredTypeGCPWIFConfig); has { + has, err := h.credStore.HasCredential(ctx, acct.ID, credentials.CredTypeGCPWIFConfig) + if err != nil { + logging.Warnf("canUseGCPFederated: credential store check failed, disabling federated: %v", err) + return false + } + if has { return false } } diff --git a/internal/api/handler_coverage_test.go b/internal/api/handler_coverage_test.go index 1750614de..baa58b648 100644 --- a/internal/api/handler_coverage_test.go +++ b/internal/api/handler_coverage_test.go @@ -17,8 +17,9 @@ func TestHandler_buildResponse_WithError(t *testing.T) { handler := &Handler{} headers := map[string]string{"Content-Type": "application/json"} - resp, err := handler.buildResponse(200, headers, nil, errors.New("test error")) - require.NoError(t, err) + // buildResponse never returns an error; when called with a non-nil err arg + // it converts it to a 500 response body so callers can use single-return form. + resp := handler.buildResponse(200, headers, nil, errors.New("test error")) assert.Equal(t, 500, resp.StatusCode) assert.Contains(t, resp.Body, "internal server error") @@ -34,8 +35,7 @@ func TestHandler_buildResponse_MarshalError(t *testing.T) { } badValue := badType{Ch: make(chan int)} - resp, err := handler.buildResponse(200, headers, badValue, nil) - require.NoError(t, err) + resp := handler.buildResponse(200, headers, badValue, nil) assert.Equal(t, 500, resp.StatusCode) assert.Contains(t, resp.Body, "internal server error") @@ -45,8 +45,7 @@ func TestHandler_buildResponse_NilBody(t *testing.T) { handler := &Handler{} headers := map[string]string{"Content-Type": "application/json"} - resp, err := handler.buildResponse(200, headers, nil, nil) - require.NoError(t, err) + resp := handler.buildResponse(200, headers, nil, nil) assert.Equal(t, 200, resp.StatusCode) // Q1 (Phase-2 UX plan): nil-body success serialises as "{}" so the diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index a725ac404..225ce356e 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -380,7 +380,10 @@ func (h *Handler) resolveCoverageByAccountKey(ctx context.Context, recs []config // resolveTargetCoverage returns the configured default coverage or 80% when // no global config is set or the configured value is zero. func (h *Handler) resolveTargetCoverage(ctx context.Context) float64 { - globalCfg, _ := h.config.GetGlobalConfig(ctx) + globalCfg, err := h.config.GetGlobalConfig(ctx) + if err != nil { + logging.Warnf("resolveTargetCoverage: could not load global config, using default 80%%: %v", err) + } if globalCfg != nil && globalCfg.DefaultCoverage > 0 { return globalCfg.DefaultCoverage } @@ -536,7 +539,10 @@ func (h *Handler) getDeploymentInfo(ctx context.Context, _ *events.LambdaFunctio // is best-effort: non-AWS deployments and STS transient failures // return "" and the frontend falls back to the "Account deleted" // warning label, which is safe. - deploymentAWSAccountID, _ := h.resolveAWSAccountID(ctx) + deploymentAWSAccountID, awsIDErr := h.resolveAWSAccountID(ctx) + if awsIDErr != nil { + logging.Debugf("getDeploymentInfo: best-effort AWS account ID resolution failed: %v", awsIDErr) + } return &DeploymentInfoResponse{ APIKeySecretURL: apiKeySecretURL, diff --git a/internal/api/handler_recommendations_refresh.go b/internal/api/handler_recommendations_refresh.go index ed5fd58ef..229a6edc4 100644 --- a/internal/api/handler_recommendations_refresh.go +++ b/internal/api/handler_recommendations_refresh.go @@ -8,6 +8,7 @@ import ( "time" "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/aws/aws-lambda-go/events" "github.com/aws/aws-sdk-go-v2/aws" awsconfig "github.com/aws/aws-sdk-go-v2/config" @@ -107,7 +108,9 @@ func (h *Handler) runMarkedCollection(ctx context.Context) (*config.Recommendati schedulerARN := os.Getenv("SCHEDULER_LAMBDA_ARN") if schedulerARN != "" { if invokeErr := h.asyncInvokeSelf(ctx, schedulerARN); invokeErr != nil { - _ = h.config.ClearCollectionStarted(ctx) + if clearErr := h.config.ClearCollectionStarted(ctx); clearErr != nil { + logging.Warnf("runMarkedCollection: failed to clear collection started marker: %v", clearErr) + } return nil, fmt.Errorf("failed to trigger async collection: %w", invokeErr) } freshness, err := h.config.GetRecommendationsFreshness(ctx) @@ -156,10 +159,13 @@ func (h *Handler) asyncInvokeSelf(ctx context.Context, functionARN string) error // internal/server/lambda.go (which checks Source == "aws.events" || // Action != "") classifies this consistently with EventBridge cron // deliveries that already exercise this code path. - payload, _ := json.Marshal(map[string]string{ + payload, marshalErr := json.Marshal(map[string]string{ "source": "aws.events", "action": "collect_recommendations", }) + if marshalErr != nil { + return fmt.Errorf("asyncInvokeSelf: failed to marshal payload: %w", marshalErr) + } _, err = invoker.Invoke(ctx, &lambda.InvokeInput{ FunctionName: aws.String(functionARN), @@ -215,7 +221,9 @@ func (h *Handler) triggerColdStartCollect(ctx context.Context) (*config.Recommen } if invokeErr := h.asyncInvokeSelf(ctx, schedulerARN); invokeErr != nil { // Roll back ONLY because we own the marker (ok==true above). - _ = h.config.ClearCollectionStarted(ctx) + if clearErr := h.config.ClearCollectionStarted(ctx); clearErr != nil { + logging.Warnf("coldStartCollect: failed to clear collection started marker: %v", clearErr) + } return nil, fmt.Errorf("failed to trigger cold-start collect: %w", invokeErr) } // Re-read freshness to return the started_at value. diff --git a/internal/api/handler_registrations.go b/internal/api/handler_registrations.go index 7969cf1fb..b9d7a8032 100644 --- a/internal/api/handler_registrations.go +++ b/internal/api/handler_registrations.go @@ -201,7 +201,10 @@ func (h *Handler) getPendingRegistration(ctx context.Context, id string) (*confi func (h *Handler) setReviewMetadata(ctx context.Context, reg *config.AccountRegistration, httpReq *events.LambdaFunctionURLRequest) { reviewedAt := time.Now() reg.ReviewedAt = &reviewedAt - session, _ := h.requireAdmin(ctx, httpReq) + session, err := h.requireAdmin(ctx, httpReq) + if err != nil { + logging.Warnf("setReviewMetadata: could not resolve admin session, reviewer ID will be unset: %v", err) + } if session != nil { reg.ReviewedBy = &session.UserID } diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index e81896455..a3449592d 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -1445,8 +1445,8 @@ func TestHandler_buildResponse_NilBodyEmitsEmptyJSONObject(t *testing.T) { h := &Handler{} headers := map[string]string{"Content-Type": "application/json"} - resp, err := h.buildResponse(200, headers, nil, nil) - require.NoError(t, err) + // buildResponse never returns an error; converts all failure modes to 500 body. + resp := h.buildResponse(200, headers, nil, nil) assert.Equal(t, 200, resp.StatusCode) assert.Equal(t, "{}", resp.Body, "nil-body success must serialise as {} so the frontend's response.json() doesn't throw") } @@ -1455,7 +1455,6 @@ func TestHandler_buildResponse_BodyMarshalsAsBefore(t *testing.T) { h := &Handler{} headers := map[string]string{"Content-Type": "application/json"} - resp, err := h.buildResponse(200, headers, map[string]string{"hello": "world"}, nil) - require.NoError(t, err) + resp := h.buildResponse(200, headers, map[string]string{"hello": "world"}, nil) assert.Equal(t, `{"hello":"world"}`, resp.Body) } diff --git a/internal/api/ri_utilization_cache.go b/internal/api/ri_utilization_cache.go index e77a22276..3631642dd 100644 --- a/internal/api/ri_utilization_cache.go +++ b/internal/api/ri_utilization_cache.go @@ -155,7 +155,7 @@ func (c *riUtilizationCache) kickBackgroundRefresh(key, region string, lookbackD } }() - _, _, _ = c.sf.Do(key, func() (any, error) { + if _, sfErr, _ := c.sf.Do(key, func() (any, error) { ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) defer cancel() @@ -166,7 +166,10 @@ func (c *riUtilizationCache) kickBackgroundRefresh(key, region string, lookbackD } c.storePayload(ctx, region, lookbackDays, data) return nil, nil - }) + }); sfErr != nil { + // error already logged inside the fetch func above + logging.Debugf("ri_utilization_cache: singleflight returned error (key=%s): %v", key, sfErr) + } }() } diff --git a/internal/api/validation.go b/internal/api/validation.go index a538db92e..460f2b59d 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -184,6 +184,15 @@ var gcpWIFConfigOptional = []string{ // (top-level keys → simple values, or one nested object for credential_source). const maxCredentialPayloadDepth = 2 +// payloadTypeMatches returns true when payload["type"] is a string equal to +// want. Returns false when the field is absent, wrong type, or wrong value. +// Extracted to avoid a `||` operator in validateCredentialPayload that would +// push the function over the cyclomatic complexity gate. +func payloadTypeMatches(payload map[string]interface{}, want string) bool { + t, ok := payload["type"].(string) + return ok && t == want +} + // validateCredentialPayload enforces shape per declared credential_type. It // rejects payloads with missing required keys, unknown extra keys, non-string // required values, or excessive nesting depth. Caller has already verified the @@ -204,7 +213,7 @@ func validateCredentialPayload(credentialType string, payload map[string]interfa if err := validateFlatPayload(credentialType, payload, gcpServiceAccountRequired, gcpServiceAccountOptional); err != nil { return err } - if t, _ := payload["type"].(string); t != "service_account" { + if !payloadTypeMatches(payload, "service_account") { return NewClientError(400, "gcp_service_account payload must have type=\"service_account\"") } return nil @@ -212,7 +221,7 @@ func validateCredentialPayload(credentialType string, payload map[string]interfa if err := validateGCPWIFPayload(payload); err != nil { return err } - if t, _ := payload["type"].(string); t != "external_account" { + if !payloadTypeMatches(payload, "external_account") { return NewClientError(400, "gcp_workload_identity_config payload must have type=\"external_account\"") } return nil diff --git a/internal/auth/service_apikeys.go b/internal/auth/service_apikeys.go index fcaced207..cae140002 100644 --- a/internal/auth/service_apikeys.go +++ b/internal/auth/service_apikeys.go @@ -281,14 +281,16 @@ func (s *Service) ValidateUserAPIKey(ctx context.Context, apiKey string) (*UserA // unbounded number of goroutines (DoS amplifier on a revoked key). keyID := key.ID go func() { - _, _, _ = s.lastUsedSFG.Do(keyID, func() (any, error) { + if _, sfErr, _ := s.lastUsedSFG.Do(keyID, func() (any, error) { updateCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() if err := s.UpdateLastUsed(updateCtx, keyID); err != nil { logging.Debugf("Failed to update API key last used timestamp for key %s: %v", keyID, err) } return nil, nil - }) + }); sfErr != nil { + logging.Debugf("service_apikeys: lastUsedSFG returned error for key %s: %v", keyID, sfErr) + } }() return key, user, nil diff --git a/internal/auth/service_mfa.go b/internal/auth/service_mfa.go index 2dc34d0c7..789834328 100644 --- a/internal/auth/service_mfa.go +++ b/internal/auth/service_mfa.go @@ -13,6 +13,7 @@ import ( "time" "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/pkg/logging" "golang.org/x/crypto/bcrypt" ) @@ -346,7 +347,9 @@ func (s *Service) validatePendingMFAEnrollment(ctx context.Context, user *User, if time.Now().After(*user.MFAPendingSecretExpiresAt) { user.MFAPendingSecret = "" user.MFAPendingSecretExpiresAt = nil - _ = s.store.UpdateUser(ctx, user) + if updateErr := s.store.UpdateUser(ctx, user); updateErr != nil { + logging.Warnf("MFAVerifyPendingCode: failed to clear expired pending secret for user: %v", updateErr) + } return fmt.Errorf("%w", ErrMFAEnrollmentExpired) } if !verifyTOTP(user.MFAPendingSecret, code) { diff --git a/internal/auth/test_helpers.go b/internal/auth/test_helpers.go index 974d66c04..88cf24cc7 100644 --- a/internal/auth/test_helpers.go +++ b/internal/auth/test_helpers.go @@ -2,6 +2,7 @@ package auth import ( "context" + "fmt" "testing" "time" @@ -20,7 +21,11 @@ func (m *MockStore) GetUserByID(ctx context.Context, userID string) (*User, erro if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*User), args.Error(1) + u, ok := args.Get(0).(*User) + if !ok { + panic(fmt.Sprintf("MockStore.GetUserByID: expected *User, got %T", args.Get(0))) + } + return u, args.Error(1) } func (m *MockStore) GetUserByEmail(ctx context.Context, email string) (*User, error) { @@ -28,7 +33,11 @@ func (m *MockStore) GetUserByEmail(ctx context.Context, email string) (*User, er if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*User), args.Error(1) + u, ok := args.Get(0).(*User) + if !ok { + panic(fmt.Sprintf("MockStore.GetUserByEmail: expected *User, got %T", args.Get(0))) + } + return u, args.Error(1) } func (m *MockStore) CreateUser(ctx context.Context, user *User) error { @@ -51,7 +60,11 @@ func (m *MockStore) ListUsers(ctx context.Context) ([]User, error) { if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]User), args.Error(1) + u, ok := args.Get(0).([]User) + if !ok { + panic(fmt.Sprintf("MockStore.ListUsers: expected []User, got %T", args.Get(0))) + } + return u, args.Error(1) } func (m *MockStore) GetUserByResetToken(ctx context.Context, token string) (*User, error) { @@ -59,7 +72,11 @@ func (m *MockStore) GetUserByResetToken(ctx context.Context, token string) (*Use if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*User), args.Error(1) + u, ok := args.Get(0).(*User) + if !ok { + panic(fmt.Sprintf("MockStore.GetUserByResetToken: expected *User, got %T", args.Get(0))) + } + return u, args.Error(1) } func (m *MockStore) AdminExists(ctx context.Context) (bool, error) { @@ -77,7 +94,11 @@ func (m *MockStore) GetGroup(ctx context.Context, groupID string) (*Group, error if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*Group), args.Error(1) + g, ok := args.Get(0).(*Group) + if !ok { + panic(fmt.Sprintf("MockStore.GetGroup: expected *Group, got %T", args.Get(0))) + } + return g, args.Error(1) } func (m *MockStore) CreateGroup(ctx context.Context, group *Group) error { @@ -100,7 +121,11 @@ func (m *MockStore) ListGroups(ctx context.Context) ([]Group, error) { if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]Group), args.Error(1) + g, ok := args.Get(0).([]Group) + if !ok { + panic(fmt.Sprintf("MockStore.ListGroups: expected []Group, got %T", args.Get(0))) + } + return g, args.Error(1) } func (m *MockStore) CountGroupMembers(ctx context.Context, groupID string) (int, error) { @@ -118,7 +143,11 @@ func (m *MockStore) GetSession(ctx context.Context, token string) (*Session, err if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*Session), args.Error(1) + s, ok := args.Get(0).(*Session) + if !ok { + panic(fmt.Sprintf("MockStore.GetSession: expected *Session, got %T", args.Get(0))) + } + return s, args.Error(1) } func (m *MockStore) DeleteSession(ctx context.Context, token string) error { @@ -147,7 +176,11 @@ func (m *MockStore) GetAPIKeyByID(ctx context.Context, keyID string) (*UserAPIKe if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*UserAPIKey), args.Error(1) + k, ok := args.Get(0).(*UserAPIKey) + if !ok { + panic(fmt.Sprintf("MockStore.GetAPIKeyByID: expected *UserAPIKey, got %T", args.Get(0))) + } + return k, args.Error(1) } func (m *MockStore) GetAPIKeyByHash(ctx context.Context, keyHash string) (*UserAPIKey, error) { @@ -155,7 +188,11 @@ func (m *MockStore) GetAPIKeyByHash(ctx context.Context, keyHash string) (*UserA if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*UserAPIKey), args.Error(1) + k, ok := args.Get(0).(*UserAPIKey) + if !ok { + panic(fmt.Sprintf("MockStore.GetAPIKeyByHash: expected *UserAPIKey, got %T", args.Get(0))) + } + return k, args.Error(1) } func (m *MockStore) ListAPIKeysByUser(ctx context.Context, userID string) ([]*UserAPIKey, error) { @@ -163,7 +200,11 @@ func (m *MockStore) ListAPIKeysByUser(ctx context.Context, userID string) ([]*Us if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]*UserAPIKey), args.Error(1) + k, ok := args.Get(0).([]*UserAPIKey) + if !ok { + panic(fmt.Sprintf("MockStore.ListAPIKeysByUser: expected []*UserAPIKey, got %T", args.Get(0))) + } + return k, args.Error(1) } func (m *MockStore) UpdateAPIKey(ctx context.Context, key *UserAPIKey) error { diff --git a/internal/commitmentopts/store_postgres.go b/internal/commitmentopts/store_postgres.go index 9d1666092..99457d0a1 100644 --- a/internal/commitmentopts/store_postgres.go +++ b/internal/commitmentopts/store_postgres.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/LeanerCloud/CUDly/internal/database" + "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgconn" ) @@ -96,7 +97,11 @@ func (s *PostgresStore) Save(ctx context.Context, combos []Combo, sourceAccountI return fmt.Errorf("begin tx: %w", err) } // Rollback is a no-op after a successful Commit. - defer func() { _ = tx.Rollback(ctx) }() + defer func() { + if rErr := tx.Rollback(ctx); rErr != nil { + logging.Warnf("commitmentopts.Save: rollback failed: %v", rErr) + } + }() if _, err := tx.Exec(ctx, `INSERT INTO commitment_options_probe_runs (singleton, probed_at, source_account_id) diff --git a/internal/credentials/cipher.go b/internal/credentials/cipher.go index ceeb7dda0..ba5a6b530 100644 --- a/internal/credentials/cipher.go +++ b/internal/credentials/cipher.go @@ -78,7 +78,11 @@ func LoadKey(ctx context.Context, resolver secrets.Resolver) (key []byte, source // migration command (cmd/rekey) which needs to detect rows encrypted under // it without going through the LoadKey env-var path. func DevKey() []byte { - k, _ := decodeHexKey(devKeyHex) + k, err := decodeHexKey(devKeyHex) + if err != nil { + // devKeyHex is a compile-time constant; an error here is a programming bug. + panic(fmt.Sprintf("credentials: invalid devKeyHex constant: %v", err)) + } return k } diff --git a/internal/credentials/resolver.go b/internal/credentials/resolver.go index cd1c8cd48..c7af8126f 100644 --- a/internal/credentials/resolver.go +++ b/internal/credentials/resolver.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "log" "os" "path/filepath" "strings" @@ -408,7 +409,11 @@ func resolveGCPWIFCredential( // federated (secret-free) account. var raw []byte if store != nil { - raw, _ = store.LoadRaw(ctx, account.ID, CredTypeGCPWIFConfig) + var loadErr error + raw, loadErr = store.LoadRaw(ctx, account.ID, CredTypeGCPWIFConfig) + if loadErr != nil { + log.Printf("credentials: LoadRaw for account %s: %v (treating as absent)", account.ID, loadErr) + } } issuer := opts.IssuerURL diff --git a/internal/database/connection.go b/internal/database/connection.go index d4177d11c..daf5c042b 100644 --- a/internal/database/connection.go +++ b/internal/database/connection.go @@ -346,7 +346,11 @@ func (c *Connection) ReleaseAdvisoryLock(ctx context.Context, lockID int64) { logging.Warnf("ReleaseAdvisoryLock called for lock %d but no pinned connection found", lockID) return } - conn := val.(*pgxpool.Conn) + conn, ok2 := val.(*pgxpool.Conn) + if !ok2 { + logging.Errorf("ReleaseAdvisoryLock: expected *pgxpool.Conn for lock %d, got %T", lockID, val) + return + } defer conn.Release() var released bool diff --git a/internal/database/postgres/migrations/migrate.go b/internal/database/postgres/migrations/migrate.go index ea2cf3728..674d222a3 100644 --- a/internal/database/postgres/migrations/migrate.go +++ b/internal/database/postgres/migrations/migrate.go @@ -433,9 +433,7 @@ func RollbackMigrations(ctx context.Context, pool *pgxpool.Pool, migrationsPath } defer m.Close() - // Log current version before rollback - currentVersion, _, _ := m.Version() - log.Printf("Rolling back %d migration(s) from version %d...", steps, currentVersion) + log.Printf("Rolling back %d migration(s)...", steps) // Rollback steps if err := m.Steps(-steps); err != nil && err != migrate.ErrNoChange { diff --git a/internal/database/postgres/testhelpers/postgres.go b/internal/database/postgres/testhelpers/postgres.go index 190e3781f..ed3b36c63 100644 --- a/internal/database/postgres/testhelpers/postgres.go +++ b/internal/database/postgres/testhelpers/postgres.go @@ -3,6 +3,7 @@ package testhelpers import ( "context" "fmt" + "log" "testing" "time" @@ -72,7 +73,9 @@ func SetupPostgresContainer(ctx context.Context, t *testing.T) (*PostgresContain // Create database connection db, err := database.NewConnection(ctx, config, nil) if err != nil { - postgresContainer.Terminate(ctx) + if termErr := postgresContainer.Terminate(ctx); termErr != nil { + log.Printf("testhelpers: failed to terminate postgres container after DB connect error: %v", termErr) + } return nil, fmt.Errorf("failed to connect to database: %w", err) } diff --git a/internal/mocks/secretsmanager.go b/internal/mocks/secretsmanager.go index b9c40e854..78719666a 100644 --- a/internal/mocks/secretsmanager.go +++ b/internal/mocks/secretsmanager.go @@ -2,6 +2,7 @@ package mocks import ( "context" + "fmt" "github.com/aws/aws-sdk-go-v2/service/secretsmanager" "github.com/stretchr/testify/mock" @@ -18,7 +19,11 @@ func (m *MockSecretsManagerClient) GetSecretValue(ctx context.Context, input *se if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*secretsmanager.GetSecretValueOutput), args.Error(1) + v, ok := args.Get(0).(*secretsmanager.GetSecretValueOutput) + if !ok { + panic(fmt.Sprintf("mock: expected *secretsmanager.GetSecretValueOutput, got %T", args.Get(0))) + } + return v, args.Error(1) } // CreateSecret mocks the CreateSecret operation @@ -27,7 +32,11 @@ func (m *MockSecretsManagerClient) CreateSecret(ctx context.Context, input *secr if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*secretsmanager.CreateSecretOutput), args.Error(1) + v, ok := args.Get(0).(*secretsmanager.CreateSecretOutput) + if !ok { + panic(fmt.Sprintf("mock: expected *secretsmanager.CreateSecretOutput, got %T", args.Get(0))) + } + return v, args.Error(1) } // UpdateSecret mocks the UpdateSecret operation @@ -36,7 +45,11 @@ func (m *MockSecretsManagerClient) UpdateSecret(ctx context.Context, input *secr if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*secretsmanager.UpdateSecretOutput), args.Error(1) + v, ok := args.Get(0).(*secretsmanager.UpdateSecretOutput) + if !ok { + panic(fmt.Sprintf("mock: expected *secretsmanager.UpdateSecretOutput, got %T", args.Get(0))) + } + return v, args.Error(1) } // SecretsManagerAPI defines the interface for Secrets Manager operations used by our code diff --git a/internal/mocks/ses.go b/internal/mocks/ses.go index 9712776a9..0b94a1c15 100644 --- a/internal/mocks/ses.go +++ b/internal/mocks/ses.go @@ -2,6 +2,7 @@ package mocks import ( "context" + "fmt" "github.com/aws/aws-sdk-go-v2/service/sesv2" "github.com/stretchr/testify/mock" @@ -18,7 +19,11 @@ func (m *MockSESClient) SendEmail(ctx context.Context, input *sesv2.SendEmailInp if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*sesv2.SendEmailOutput), args.Error(1) + v, ok := args.Get(0).(*sesv2.SendEmailOutput) + if !ok { + panic(fmt.Sprintf("mock: expected *sesv2.SendEmailOutput, got %T", args.Get(0))) + } + return v, args.Error(1) } // SESAPI defines the interface for SES operations used by our code diff --git a/internal/mocks/sns.go b/internal/mocks/sns.go index 5bae0d212..87ce6fe9a 100644 --- a/internal/mocks/sns.go +++ b/internal/mocks/sns.go @@ -2,6 +2,7 @@ package mocks import ( "context" + "fmt" "github.com/aws/aws-sdk-go-v2/service/sns" "github.com/stretchr/testify/mock" @@ -18,7 +19,11 @@ func (m *MockSNSClient) Publish(ctx context.Context, input *sns.PublishInput, op if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*sns.PublishOutput), args.Error(1) + v, ok := args.Get(0).(*sns.PublishOutput) + if !ok { + panic(fmt.Sprintf("mock: expected *sns.PublishOutput, got %T", args.Get(0))) + } + return v, args.Error(1) } // SNSAPI defines the interface for SNS operations used by our code diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index e6b32b4e6..f9f6b6401 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -2,6 +2,7 @@ package mocks import ( "context" + "fmt" "time" "github.com/LeanerCloud/CUDly/internal/auth" @@ -63,7 +64,11 @@ func (m *MockConfigStore) GetGlobalConfig(ctx context.Context) (*config.GlobalCo if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.GlobalConfig), args.Error(1) + v, ok := args.Get(0).(*config.GlobalConfig) + if !ok { + panic(fmt.Sprintf("mock: expected *config.GlobalConfig, got %T", args.Get(0))) + } + return v, args.Error(1) } // SaveGlobalConfig mocks the SaveGlobalConfig operation @@ -78,7 +83,11 @@ func (m *MockConfigStore) GetServiceConfig(ctx context.Context, provider, servic if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.ServiceConfig), args.Error(1) + v, ok := args.Get(0).(*config.ServiceConfig) + if !ok { + panic(fmt.Sprintf("mock: expected *config.ServiceConfig, got %T", args.Get(0))) + } + return v, args.Error(1) } // SaveServiceConfig mocks the SaveServiceConfig operation @@ -93,7 +102,11 @@ func (m *MockConfigStore) ListServiceConfigs(ctx context.Context) ([]config.Serv if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.ServiceConfig), args.Error(1) + v, ok := args.Get(0).([]config.ServiceConfig) + if !ok { + panic(fmt.Sprintf("mock: expected []config.ServiceConfig, got %T", args.Get(0))) + } + return v, args.Error(1) } // CreatePurchasePlan mocks the CreatePurchasePlan operation @@ -117,7 +130,11 @@ func (m *MockConfigStore) GetPurchasePlan(ctx context.Context, planID string) (* if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.PurchasePlan), args.Error(1) + v, ok := args.Get(0).(*config.PurchasePlan) + if !ok { + panic(fmt.Sprintf("mock: expected *config.PurchasePlan, got %T", args.Get(0))) + } + return v, args.Error(1) } // UpdatePurchasePlan mocks the UpdatePurchasePlan operation @@ -156,7 +173,11 @@ func (m *MockConfigStore) ListPurchasePlans(ctx context.Context, filter config.P if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.PurchasePlan), args.Error(1) + v, ok := args.Get(0).([]config.PurchasePlan) + if !ok { + panic(fmt.Sprintf("mock: expected []config.PurchasePlan, got %T", args.Get(0))) + } + return v, args.Error(1) } // SavePurchaseExecution mocks the SavePurchaseExecution operation. @@ -175,7 +196,11 @@ func (m *MockConfigStore) TransitionExecutionStatus(ctx context.Context, executi if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.PurchaseExecution), args.Error(1) + v, ok := args.Get(0).(*config.PurchaseExecution) + if !ok { + panic(fmt.Sprintf("mock: expected *config.PurchaseExecution, got %T", args.Get(0))) + } + return v, args.Error(1) } // CancelExecutionAtomic mocks the CancelExecutionAtomic operation. @@ -212,7 +237,11 @@ func (m *MockConfigStore) GetPendingExecutions(ctx context.Context) ([]config.Pu if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.PurchaseExecution), args.Error(1) + v, ok := args.Get(0).([]config.PurchaseExecution) + if !ok { + panic(fmt.Sprintf("mock: expected []config.PurchaseExecution, got %T", args.Get(0))) + } + return v, args.Error(1) } // GetExecutionByID mocks the GetExecutionByID operation @@ -221,7 +250,11 @@ func (m *MockConfigStore) GetExecutionByID(ctx context.Context, executionID stri if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.PurchaseExecution), args.Error(1) + v, ok := args.Get(0).(*config.PurchaseExecution) + if !ok { + panic(fmt.Sprintf("mock: expected *config.PurchaseExecution, got %T", args.Get(0))) + } + return v, args.Error(1) } // GetExecutionByPlanAndDate mocks the GetExecutionByPlanAndDate operation @@ -230,7 +263,11 @@ func (m *MockConfigStore) GetExecutionByPlanAndDate(ctx context.Context, planID if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.PurchaseExecution), args.Error(1) + v, ok := args.Get(0).(*config.PurchaseExecution) + if !ok { + panic(fmt.Sprintf("mock: expected *config.PurchaseExecution, got %T", args.Get(0))) + } + return v, args.Error(1) } // CountPendingExecutionsForAccount mocks the CountPendingExecutionsForAccount operation. @@ -259,7 +296,11 @@ func (m *MockConfigStore) ListPendingExecutionIDsForAccount(ctx context.Context, if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]string), args.Error(1) + v, ok := args.Get(0).([]string) + if !ok { + panic(fmt.Sprintf("mock: expected []string, got %T", args.Get(0))) + } + return v, args.Error(1) } // SavePurchaseHistory mocks the SavePurchaseHistory operation @@ -274,7 +315,11 @@ func (m *MockConfigStore) GetPurchaseHistory(ctx context.Context, accountID stri if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) + v, ok := args.Get(0).([]config.PurchaseHistoryRecord) + if !ok { + panic(fmt.Sprintf("mock: expected []config.PurchaseHistoryRecord, got %T", args.Get(0))) + } + return v, args.Error(1) } // GetAllPurchaseHistory mocks the GetAllPurchaseHistory operation @@ -283,7 +328,11 @@ func (m *MockConfigStore) GetAllPurchaseHistory(ctx context.Context, limit int) if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) + v, ok := args.Get(0).([]config.PurchaseHistoryRecord) + if !ok { + panic(fmt.Sprintf("mock: expected []config.PurchaseHistoryRecord, got %T", args.Get(0))) + } + return v, args.Error(1) } // GetActivePurchaseHistory mocks the GetActivePurchaseHistory operation @@ -292,7 +341,11 @@ func (m *MockConfigStore) GetActivePurchaseHistory(ctx context.Context, asOf tim if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) + v, ok := args.Get(0).([]config.PurchaseHistoryRecord) + if !ok { + panic(fmt.Sprintf("mock: expected []config.PurchaseHistoryRecord, got %T", args.Get(0))) + } + return v, args.Error(1) } // GetPurchaseHistoryFiltered mocks the GetPurchaseHistoryFiltered operation (issue #701). @@ -301,7 +354,11 @@ func (m *MockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, filter if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) + v, ok := args.Get(0).([]config.PurchaseHistoryRecord) + if !ok { + panic(fmt.Sprintf("mock: expected []config.PurchaseHistoryRecord, got %T", args.Get(0))) + } + return v, args.Error(1) } // GetPurchaseHistoryByPurchaseID mocks the GetPurchaseHistoryByPurchaseID operation (issue #290). @@ -310,7 +367,11 @@ func (m *MockConfigStore) GetPurchaseHistoryByPurchaseID(ctx context.Context, pu if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.PurchaseHistoryRecord), args.Error(1) + v, ok := args.Get(0).(*config.PurchaseHistoryRecord) + if !ok { + panic(fmt.Sprintf("mock: expected *config.PurchaseHistoryRecord, got %T", args.Get(0))) + } + return v, args.Error(1) } // MarkPurchaseRevoked mocks the MarkPurchaseRevoked operation (issue #290). @@ -347,7 +408,11 @@ func (m *MockConfigStore) GetPurchaseHistoryInFlight(ctx context.Context) ([]*co if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]*config.PurchaseHistoryRecord), args.Error(1) + v, ok := args.Get(0).([]*config.PurchaseHistoryRecord) + if !ok { + panic(fmt.Sprintf("mock: expected []*config.PurchaseHistoryRecord, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) SaveRIExchangeRecord(ctx context.Context, record *config.RIExchangeRecord) error { @@ -360,7 +425,11 @@ func (m *MockConfigStore) GetRIExchangeRecord(ctx context.Context, id string) (* if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.RIExchangeRecord), args.Error(1) + v, ok := args.Get(0).(*config.RIExchangeRecord) + if !ok { + panic(fmt.Sprintf("mock: expected *config.RIExchangeRecord, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) GetRIExchangeRecordByToken(ctx context.Context, token string) (*config.RIExchangeRecord, error) { @@ -368,7 +437,11 @@ func (m *MockConfigStore) GetRIExchangeRecordByToken(ctx context.Context, token if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.RIExchangeRecord), args.Error(1) + v, ok := args.Get(0).(*config.RIExchangeRecord) + if !ok { + panic(fmt.Sprintf("mock: expected *config.RIExchangeRecord, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) GetRIExchangeHistory(ctx context.Context, since time.Time, limit int) ([]config.RIExchangeRecord, error) { @@ -376,7 +449,11 @@ func (m *MockConfigStore) GetRIExchangeHistory(ctx context.Context, since time.T if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.RIExchangeRecord), args.Error(1) + v, ok := args.Get(0).([]config.RIExchangeRecord) + if !ok { + panic(fmt.Sprintf("mock: expected []config.RIExchangeRecord, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string, actor *string) (*config.RIExchangeRecord, error) { @@ -384,7 +461,11 @@ func (m *MockConfigStore) TransitionRIExchangeStatus(ctx context.Context, id str if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.RIExchangeRecord), args.Error(1) + v, ok := args.Get(0).(*config.RIExchangeRecord) + if !ok { + panic(fmt.Sprintf("mock: expected *config.RIExchangeRecord, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) CompleteRIExchange(ctx context.Context, id string, exchangeID string) error { @@ -404,7 +485,11 @@ func (m *MockConfigStore) GetRIExchangeDailySpend(ctx context.Context, date time func (m *MockConfigStore) CancelAllPendingExchanges(ctx context.Context) (int64, error) { args := m.Called(ctx) - return args.Get(0).(int64), args.Error(1) + v, ok := args.Get(0).(int64) + if !ok { + panic(fmt.Sprintf("mock: expected int64, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) GetStaleProcessingExchanges(ctx context.Context, olderThan time.Duration) ([]config.RIExchangeRecord, error) { @@ -412,7 +497,11 @@ func (m *MockConfigStore) GetStaleProcessingExchanges(ctx context.Context, older if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.RIExchangeRecord), args.Error(1) + v, ok := args.Get(0).([]config.RIExchangeRecord) + if !ok { + panic(fmt.Sprintf("mock: expected []config.RIExchangeRecord, got %T", args.Get(0))) + } + return v, args.Error(1) } // MockAuthStore is a mock implementation of auth.Store @@ -426,7 +515,11 @@ func (m *MockAuthStore) GetUserByID(ctx context.Context, userID string) (*auth.U if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*auth.User), args.Error(1) + v, ok := args.Get(0).(*auth.User) + if !ok { + panic(fmt.Sprintf("mock: expected *auth.User, got %T", args.Get(0))) + } + return v, args.Error(1) } // GetUserByEmail mocks the GetUserByEmail operation @@ -435,7 +528,11 @@ func (m *MockAuthStore) GetUserByEmail(ctx context.Context, email string) (*auth if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*auth.User), args.Error(1) + v, ok := args.Get(0).(*auth.User) + if !ok { + panic(fmt.Sprintf("mock: expected *auth.User, got %T", args.Get(0))) + } + return v, args.Error(1) } // CreateUser mocks the CreateUser operation @@ -462,7 +559,11 @@ func (m *MockAuthStore) ListUsers(ctx context.Context) ([]auth.User, error) { if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]auth.User), args.Error(1) + v, ok := args.Get(0).([]auth.User) + if !ok { + panic(fmt.Sprintf("mock: expected []auth.User, got %T", args.Get(0))) + } + return v, args.Error(1) } // GetUserByResetToken mocks the GetUserByResetToken operation @@ -471,7 +572,11 @@ func (m *MockAuthStore) GetUserByResetToken(ctx context.Context, token string) ( if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*auth.User), args.Error(1) + v, ok := args.Get(0).(*auth.User) + if !ok { + panic(fmt.Sprintf("mock: expected *auth.User, got %T", args.Get(0))) + } + return v, args.Error(1) } // AdminExists mocks the AdminExists operation @@ -492,7 +597,11 @@ func (m *MockAuthStore) GetGroup(ctx context.Context, groupID string) (*auth.Gro if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*auth.Group), args.Error(1) + v, ok := args.Get(0).(*auth.Group) + if !ok { + panic(fmt.Sprintf("mock: expected *auth.Group, got %T", args.Get(0))) + } + return v, args.Error(1) } // CreateGroup mocks the CreateGroup operation @@ -519,7 +628,11 @@ func (m *MockAuthStore) ListGroups(ctx context.Context) ([]auth.Group, error) { if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]auth.Group), args.Error(1) + v, ok := args.Get(0).([]auth.Group) + if !ok { + panic(fmt.Sprintf("mock: expected []auth.Group, got %T", args.Get(0))) + } + return v, args.Error(1) } // CountGroupMembers mocks the CountGroupMembers operation @@ -540,7 +653,11 @@ func (m *MockAuthStore) GetSession(ctx context.Context, token string) (*auth.Ses if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*auth.Session), args.Error(1) + v, ok := args.Get(0).(*auth.Session) + if !ok { + panic(fmt.Sprintf("mock: expected *auth.Session, got %T", args.Get(0))) + } + return v, args.Error(1) } // DeleteSession mocks the DeleteSession operation @@ -575,7 +692,11 @@ func (m *MockAuthStore) GetAPIKeyByID(ctx context.Context, keyID string) (*auth. if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*auth.UserAPIKey), args.Error(1) + v, ok := args.Get(0).(*auth.UserAPIKey) + if !ok { + panic(fmt.Sprintf("mock: expected *auth.UserAPIKey, got %T", args.Get(0))) + } + return v, args.Error(1) } // GetAPIKeyByHash mocks the GetAPIKeyByHash operation @@ -584,7 +705,11 @@ func (m *MockAuthStore) GetAPIKeyByHash(ctx context.Context, keyHash string) (*a if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*auth.UserAPIKey), args.Error(1) + v, ok := args.Get(0).(*auth.UserAPIKey) + if !ok { + panic(fmt.Sprintf("mock: expected *auth.UserAPIKey, got %T", args.Get(0))) + } + return v, args.Error(1) } // ListAPIKeysByUser mocks the ListAPIKeysByUser operation @@ -593,7 +718,11 @@ func (m *MockAuthStore) ListAPIKeysByUser(ctx context.Context, userID string) ([ if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]*auth.UserAPIKey), args.Error(1) + v, ok := args.Get(0).([]*auth.UserAPIKey) + if !ok { + panic(fmt.Sprintf("mock: expected []*auth.UserAPIKey, got %T", args.Get(0))) + } + return v, args.Error(1) } // UpdateAPIKey mocks the UpdateAPIKey operation @@ -647,7 +776,11 @@ func (m *MockConfigStore) GetCloudAccount(ctx context.Context, id string) (*conf if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.CloudAccount), args.Error(1) + v, ok := args.Get(0).(*config.CloudAccount) + if !ok { + panic(fmt.Sprintf("mock: expected *config.CloudAccount, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) GetCloudAccountByExternalID(ctx context.Context, provider, externalID string) (*config.CloudAccount, error) { @@ -661,7 +794,11 @@ func (m *MockConfigStore) GetCloudAccountByExternalID(ctx context.Context, provi if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.CloudAccount), args.Error(1) + v, ok := args.Get(0).(*config.CloudAccount) + if !ok { + panic(fmt.Sprintf("mock: expected *config.CloudAccount, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) UpdateCloudAccount(ctx context.Context, account *config.CloudAccount) error { @@ -694,7 +831,11 @@ func (m *MockConfigStore) ListCloudAccounts(ctx context.Context, filter config.C if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.CloudAccount), args.Error(1) + v, ok := args.Get(0).([]config.CloudAccount) + if !ok { + panic(fmt.Sprintf("mock: expected []config.CloudAccount, got %T", args.Get(0))) + } + return v, args.Error(1) } // Account credentials @@ -741,7 +882,11 @@ func (m *MockConfigStore) GetAccountServiceOverride(ctx context.Context, account if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.AccountServiceOverride), args.Error(1) + v, ok := args.Get(0).(*config.AccountServiceOverride) + if !ok { + panic(fmt.Sprintf("mock: expected *config.AccountServiceOverride, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) SaveAccountServiceOverride(ctx context.Context, override *config.AccountServiceOverride) error { @@ -771,7 +916,11 @@ func (m *MockConfigStore) ListAccountServiceOverrides(ctx context.Context, accou if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.AccountServiceOverride), args.Error(1) + v, ok := args.Get(0).([]config.AccountServiceOverride) + if !ok { + panic(fmt.Sprintf("mock: expected []config.AccountServiceOverride, got %T", args.Get(0))) + } + return v, args.Error(1) } // Plan ↔ account association @@ -798,13 +947,21 @@ func (m *MockConfigStore) GetPlanAccounts(ctx context.Context, planID string) ([ if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.CloudAccount), args.Error(1) + v, ok := args.Get(0).([]config.CloudAccount) + if !ok { + panic(fmt.Sprintf("mock: expected []config.CloudAccount, got %T", args.Get(0))) + } + return v, args.Error(1) } // CleanupOldExecutions mocks the CleanupOldExecutions operation func (m *MockConfigStore) CleanupOldExecutions(ctx context.Context, retentionDays int) (int64, error) { args := m.Called(ctx, retentionDays) - return args.Get(0).(int64), args.Error(1) + v, ok := args.Get(0).(int64) + if !ok { + panic(fmt.Sprintf("mock: expected int64, got %T", args.Get(0))) + } + return v, args.Error(1) } // Recommendations cache @@ -837,7 +994,11 @@ func (m *MockConfigStore) ListStoredRecommendations(ctx context.Context, filter if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.RecommendationRecord), args.Error(1) + v, ok := args.Get(0).([]config.RecommendationRecord) + if !ok { + panic(fmt.Sprintf("mock: expected []config.RecommendationRecord, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) GetRecommendationsFreshness(ctx context.Context) (*config.RecommendationsFreshness, error) { @@ -848,7 +1009,11 @@ func (m *MockConfigStore) GetRecommendationsFreshness(ctx context.Context) (*con if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.RecommendationsFreshness), args.Error(1) + v, ok := args.Get(0).(*config.RecommendationsFreshness) + if !ok { + panic(fmt.Sprintf("mock: expected *config.RecommendationsFreshness, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) SetRecommendationsCollectionError(ctx context.Context, errMsg string) error { @@ -867,7 +1032,11 @@ func (m *MockConfigStore) GetRIUtilizationCache(ctx context.Context, region stri if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.RIUtilizationCacheEntry), args.Error(1) + v, ok := args.Get(0).(*config.RIUtilizationCacheEntry) + if !ok { + panic(fmt.Sprintf("mock: expected *config.RIUtilizationCacheEntry, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) UpsertRIUtilizationCache(ctx context.Context, region string, lookbackDays int, payload []byte, fetchedAt time.Time) error { @@ -888,7 +1057,11 @@ func (m *MockConfigStore) GetAccountRegistration(ctx context.Context, id string) if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.AccountRegistration), args.Error(1) + v, ok := args.Get(0).(*config.AccountRegistration) + if !ok { + panic(fmt.Sprintf("mock: expected *config.AccountRegistration, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) GetAccountRegistrationByToken(ctx context.Context, token string) (*config.AccountRegistration, error) { @@ -896,7 +1069,11 @@ func (m *MockConfigStore) GetAccountRegistrationByToken(ctx context.Context, tok if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).(*config.AccountRegistration), args.Error(1) + v, ok := args.Get(0).(*config.AccountRegistration) + if !ok { + panic(fmt.Sprintf("mock: expected *config.AccountRegistration, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) ListAccountRegistrations(ctx context.Context, filter config.AccountRegistrationFilter) ([]config.AccountRegistration, error) { @@ -904,7 +1081,11 @@ func (m *MockConfigStore) ListAccountRegistrations(ctx context.Context, filter c if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.AccountRegistration), args.Error(1) + v, ok := args.Get(0).([]config.AccountRegistration) + if !ok { + panic(fmt.Sprintf("mock: expected []config.AccountRegistration, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) UpdateAccountRegistration(ctx context.Context, reg *config.AccountRegistration) error { @@ -966,7 +1147,11 @@ func (m *MockConfigStore) ListActiveSuppressions(ctx context.Context) ([]config. if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.PurchaseSuppression), args.Error(1) + v, ok := args.Get(0).([]config.PurchaseSuppression) + if !ok { + panic(fmt.Sprintf("mock: expected []config.PurchaseSuppression, got %T", args.Get(0))) + } + return v, args.Error(1) } // GetPendingExecutionsTx mocks the GetPendingExecutionsTx operation. @@ -981,7 +1166,11 @@ func (m *MockConfigStore) GetPendingExecutionsTx(ctx context.Context, tx pgx.Tx) if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.PurchaseExecution), args.Error(1) + v, ok := args.Get(0).([]config.PurchaseExecution) + if !ok { + panic(fmt.Sprintf("mock: expected []config.PurchaseExecution, got %T", args.Get(0))) + } + return v, args.Error(1) } func (m *MockConfigStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx, execution *config.PurchaseExecution) error { @@ -1009,7 +1198,11 @@ func (m *MockConfigStore) GetExecutionsByStatuses(ctx context.Context, statuses if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.PurchaseExecution), args.Error(1) + v, ok := args.Get(0).([]config.PurchaseExecution) + if !ok { + panic(fmt.Sprintf("mock: expected []config.PurchaseExecution, got %T", args.Get(0))) + } + return v, args.Error(1) } // GetPlannedExecutions mocks the GetPlannedExecutions operation. @@ -1018,7 +1211,11 @@ func (m *MockConfigStore) GetPlannedExecutions(ctx context.Context, statuses []s if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.PurchaseExecution), args.Error(1) + v, ok := args.Get(0).([]config.PurchaseExecution) + if !ok { + panic(fmt.Sprintf("mock: expected []config.PurchaseExecution, got %T", args.Get(0))) + } + return v, args.Error(1) } // GetStaleApprovedExecutions mocks the GetStaleApprovedExecutions operation. @@ -1027,7 +1224,11 @@ func (m *MockConfigStore) GetStaleApprovedExecutions(ctx context.Context, olderT if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.PurchaseExecution), args.Error(1) + v, ok := args.Get(0).([]config.PurchaseExecution) + if !ok { + panic(fmt.Sprintf("mock: expected []config.PurchaseExecution, got %T", args.Get(0))) + } + return v, args.Error(1) } // ListStuckExecutions mocks the ListStuckExecutions operation. @@ -1036,7 +1237,11 @@ func (m *MockConfigStore) ListStuckExecutions(ctx context.Context, statuses []st if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.PurchaseExecution), args.Error(1) + v, ok := args.Get(0).([]config.PurchaseExecution) + if !ok { + panic(fmt.Sprintf("mock: expected []config.PurchaseExecution, got %T", args.Get(0))) + } + return v, args.Error(1) } // GetScheduledExecutionsDue mocks the GetScheduledExecutionsDue operation. @@ -1050,7 +1255,11 @@ func (m *MockConfigStore) GetScheduledExecutionsDue(ctx context.Context) ([]conf if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]config.PurchaseExecution), args.Error(1) + v, ok := args.Get(0).([]config.PurchaseExecution) + if !ok { + panic(fmt.Sprintf("mock: expected []config.PurchaseExecution, got %T", args.Get(0))) + } + return v, args.Error(1) } // MarkCollectionStarted mocks the MarkCollectionStarted operation. diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index a5a43b890..5ecb09843 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -195,6 +195,15 @@ func (m *Manager) executeMultiAccount(ctx context.Context, baseExec *config.Purc return fmt.Errorf("%w: %s", errAllAccountsFailed, strings.Join(errs, "; ")) } +// saveExecutionStatusBestEffort saves the execution record and logs any error. +// Used in error paths where we need to persist the failure status but cannot +// propagate the save error (the original error is already being returned). +func (m *Manager) saveExecutionStatusBestEffort(ctx context.Context, exec *config.PurchaseExecution) { + if err := m.config.SavePurchaseExecution(ctx, exec); err != nil { + logging.Warnf("execution: failed to persist error status for account %v: %v", exec.CloudAccountID, err) + } +} + // executeForAccount runs a single plan execution for one cloud account. // It creates a new PurchaseExecution record tagged with cloud_account_id, resolves // per-account credentials, executes purchases, and saves the result. @@ -229,7 +238,7 @@ func (m *Manager) executeForAccount(ctx context.Context, baseExec *config.Purcha if err != nil { acctExec.Status = "failed" acctExec.Error = err.Error() - _ = m.config.SavePurchaseExecution(ctx, &acctExec) + m.saveExecutionStatusBestEffort(ctx, &acctExec) return false, fmt.Errorf("credential resolution failed for account %s: %w", account.ID, err) } diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 203a6241a..b0300f5ba 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -349,7 +349,10 @@ func (s *Scheduler) collectAllProviders(ctx context.Context, globalCfg *config.G // goroutine returns nil. After Wait, propagate ctx cancellation so // callers can distinguish "all providers completed" from "the parent // ctx was canceled mid-fan-out". - _ = g.Wait() + if waitErr := g.Wait(); waitErr != nil { + // Goroutines return nil to isolate per-provider failures; non-nil is unexpected. + logging.Warnf("collectAllProviders: errgroup.Wait returned unexpected error: %v", waitErr) + } if cerr := ctx.Err(); cerr != nil { return nil, 0, nil, nil, nil, cerr } @@ -567,7 +570,10 @@ func fanOutPerAccount( return nil }) } - _ = g.Wait() // errs are always nil (swallowed above) + if waitErr := g.Wait(); waitErr != nil { + // Goroutines return nil to isolate per-account failures; non-nil is unexpected. + logging.Warnf("fanOutPerAccount: errgroup.Wait returned unexpected error: %v", waitErr) + } return all, outcome } @@ -898,7 +904,11 @@ func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider, PaymentOption: globalCfg.DefaultPayment, LookbackPeriod: fmt.Sprintf("%dd", lookbackDays), } - recs, _ = recClient.GetRecommendations(ctx, params) + var recErr error + recs, recErr = recClient.GetRecommendations(ctx, params) + if recErr != nil { + logging.Warnf("fetchAndConvert: %s GetRecommendations fallback failed: %v", providerName, recErr) + } } result := s.convertRecommendations(recs, providerName) if accountID != nil { diff --git a/internal/server/http.go b/internal/server/http.go index 45d349665..0e6f4878e 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -400,5 +400,7 @@ func lambdaResponseToHTTP(w http.ResponseWriter, lambdaResp *events.LambdaFuncti // Set status code and write body w.WriteHeader(lambdaResp.StatusCode) - w.Write(body) + if _, err := w.Write(body); err != nil { + log.Printf("http: failed to write response body: %v", err) + } } From 212aa17007e490a6da07375a4ef33fb46a1de0b3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 3 Jul 2026 15:30:17 +0200 Subject: [PATCH 3/5] fix(config): fail loud on missing execution and seed FK accounts in tests Fixes the three integration tests failing on main since 2026-06-11 (issue #1339): - TestPostgresStore_PurchaseExecutions/Get_execution_by_ID_-_not_found expected an error but GetExecutionByID returned (nil, nil) for a missing row. That silent-nil contract violates the fail-loud policy and forced every caller to nil-check. GetExecutionByID now returns an error wrapping config.ErrNotFound; a nil error guarantees a non-nil execution. All callers (handler_purchases, revoke, scoping, purchase approvals/messages, store-internal CAS probes) switched to errors.Is(err, config.ErrNotFound) and their dead nil-checks were removed. Mock fixtures that simulated not-found via (nil, nil) now return the sentinel error. The pgxmock unit test TestGetExecutionByID_NotFound already asserted the error contract. - TestPostgresStore_UpsertRecommendations_AccountScopedEviction and TestPostgresStore_UpsertRecommendations_AmbientAndRegisteredCoexist failed with an FK violation: recommendations.cloud_account_id REFERENCES cloud_accounts(id) (migration 000030) but the tests never created the referenced accounts. Production upserts only carry registered account IDs, so the tests now seed the cloud accounts first via a new seedRecommendationCloudAccount helper. --- internal/api/handler_purchases.go | 37 +++++++++---------- internal/api/handler_purchases_revoke.go | 8 ++-- internal/api/handler_purchases_revoke_test.go | 28 +++++++------- internal/api/handler_purchases_test.go | 17 +++++---- internal/api/scoping.go | 7 ++-- internal/config/store_postgres.go | 19 ++++------ internal/config/store_postgres_db_test.go | 7 ++-- .../store_postgres_recommendations_test.go | 25 ++++++++++++- internal/purchase/approvals.go | 13 ++++--- internal/purchase/approvals_test.go | 5 ++- internal/purchase/messages.go | 13 ++++--- internal/purchase/messages_test.go | 2 +- 12 files changed, 104 insertions(+), 77 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index fd6e99fe7..09a11a762 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -239,12 +239,12 @@ func (h *Handler) authorizeExecutionManagement(ctx context.Context, session *Ses } execution, err := h.config.GetExecutionByID(ctx, executionID) + if errors.Is(err, config.ErrNotFound) { + return errNotFound + } if err != nil { return fmt.Errorf("failed to get execution: %w", err) } - if execution == nil { - return errNotFound - } // Creator match: both IDs must be non-empty and equal. An empty-string // collision (legacy NULL creator + missing session UserID) must not @@ -381,12 +381,12 @@ func (h *Handler) cancelOrRecoverExecution(ctx context.Context, executionID stri return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: %v", executionID, err)) } existing, getErr := h.config.GetExecutionByID(ctx, executionID) + if errors.Is(getErr, config.ErrNotFound) { + return nil, NewClientError(404, fmt.Sprintf("execution %s not found", executionID)) + } if getErr != nil { return nil, fmt.Errorf("disable plan: failed to get execution %s after conflict: %w", executionID, getErr) } - if existing == nil { - return nil, NewClientError(404, fmt.Sprintf("execution %s not found", executionID)) - } if existing.Status != "cancelled" { return nil, NewClientError(409, fmt.Sprintf( "execution %s cannot be cancelled (status=%s)", @@ -432,12 +432,12 @@ func (h *Handler) disablePlan(ctx context.Context, planID string) error { // threshold — same pattern as loadCancelableExecution in the purchase package. func (h *Handler) loadApproveExecution(ctx context.Context, execID string) (*config.PurchaseExecution, error) { execution, err := h.config.GetExecutionByID(ctx, execID) + if errors.Is(err, config.ErrNotFound) { + return nil, NewClientError(404, "execution not found") + } if err != nil { return nil, fmt.Errorf("failed to get execution: %w", err) } - if execution == nil { - return nil, NewClientError(404, "execution not found") - } // Preflight (issue #609): reject non-AWS orphan executions before the // cloud SDK is reached. Delegates to the centralized predicate in the // purchase package so the logic is maintained in one place. @@ -830,12 +830,12 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction } execution, err := h.config.GetExecutionByID(ctx, execID) + if errors.Is(err, config.ErrNotFound) { + return nil, NewClientError(404, "execution not found") + } if err != nil { return nil, fmt.Errorf("failed to get execution: %w", err) } - if execution == nil { - return nil, NewClientError(404, "execution not found") - } // Three-mode dispatch: // 1. Session present AND RBAC-authorized (admin / cancel-any / @@ -1150,12 +1150,12 @@ func (h *Handler) loadAndValidateRetryRequest(ctx context.Context, req *events.L } failedExec, err := h.config.GetExecutionByID(ctx, execID) + if errors.Is(err, config.ErrNotFound) { + return nil, nil, NewClientError(404, "execution not found") + } if err != nil { return nil, nil, fmt.Errorf("failed to get execution: %w", err) } - if failedExec == nil { - return nil, nil, NewClientError(404, "execution not found") - } session, err := h.requireSession(ctx, req) if err != nil { @@ -1455,16 +1455,15 @@ func (h *Handler) getPurchaseDetails(ctx context.Context, req *events.LambdaFunc } execution, err := h.config.GetExecutionByID(ctx, executionID) + if errors.Is(err, config.ErrNotFound) { + return nil, NewClientError(404, "execution not found") + } if err != nil { // Real DB failure (connection error, timeout, etc.) - surface as 500 // so infrastructure issues are visible to operators and not silently // masked as 404s (issue #976). return nil, fmt.Errorf("failed to get execution: %w", err) } - if execution == nil { - // GetExecutionByID returns (nil, nil) for a missing row (issue #976). - return nil, NewClientError(404, "execution not found") - } // Scope: reject if the execution's plan isn't accessible to the session. if err := h.requirePlanAccess(ctx, session, execution.PlanID); err != nil { diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go index f0f614196..2f9827fd8 100644 --- a/internal/api/handler_purchases_revoke.go +++ b/internal/api/handler_purchases_revoke.go @@ -156,12 +156,14 @@ func (h *Handler) revokePurchase(ctx context.Context, req *events.LambdaFunction // row straight to revokeScheduledExecution which calls CancelScheduledExecutionAtomic // (WHERE status='scheduled' CAS) and returns 410 if the scheduler already fired. // - // A genuine DB error from GetExecutionByID surfaces as 500; (nil, nil) means + // A genuine DB error from GetExecutionByID surfaces as 500; ErrNotFound means // the ID is not an execution (or is not yet visible) and we fall through to // the purchase_history lookup below. - if execution, execErr := h.config.GetExecutionByID(ctx, purchaseID); execErr != nil { + execution, execErr := h.config.GetExecutionByID(ctx, purchaseID) + if execErr != nil && !errors.Is(execErr, config.ErrNotFound) { return nil, fmt.Errorf("revoke: GetExecutionByID %s: %w", purchaseID, execErr) - } else if execution != nil { + } + if execErr == nil { return h.revokeScheduledExecution(ctx, session, execution) } diff --git a/internal/api/handler_purchases_revoke_test.go b/internal/api/handler_purchases_revoke_test.go index 678b3ba02..4f3a8fae2 100644 --- a/internal/api/handler_purchases_revoke_test.go +++ b/internal/api/handler_purchases_revoke_test.go @@ -120,8 +120,8 @@ func TestRevokePurchase_PurchaseNotFound(t *testing.T) { adminSess := revokeAdminSession() mockAuth.On("ValidateSession", ctx, "tok").Return(adminSess, nil) - // GetExecutionByID returns (nil, nil): not an execution row, fall through to history. - mockStore.On("GetExecutionByID", ctx, "pid-1").Return((*config.PurchaseExecution)(nil), nil) + // GetExecutionByID returns ErrNotFound: not an execution row, fall through to history. + mockStore.On("GetExecutionByID", ctx, "pid-1").Return(nil, fmt.Errorf("%w: execution pid-1", config.ErrNotFound)) mockStore.On("GetPurchaseHistoryByPurchaseID", ctx, "pid-1").Return((*config.PurchaseHistoryRecord)(nil), nil) h := &Handler{config: mockStore, auth: mockAuth} @@ -149,8 +149,8 @@ func TestRevokePurchase_AlreadyRevoked(t *testing.T) { r := armReservationRecord() r.RevokedAt = &revokedAt r.RevokedVia = "direct-api" - // GetExecutionByID returns (nil, nil): not an execution row, fall through to history. - mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return((*config.PurchaseExecution)(nil), nil) + // GetExecutionByID returns ErrNotFound: not an execution row, fall through to history. + mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return(nil, fmt.Errorf("%w: execution %s", config.ErrNotFound, r.PurchaseID)) mockStore.On("GetPurchaseHistoryByPurchaseID", ctx, r.PurchaseID).Return(r, nil) h := &Handler{config: mockStore, auth: mockAuth} @@ -177,8 +177,8 @@ func TestRevokePurchase_AWSReturns422(t *testing.T) { r := armReservationRecord() r.Provider = "aws" - // GetExecutionByID returns (nil, nil): not an execution row, fall through to history. - mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return((*config.PurchaseExecution)(nil), nil) + // GetExecutionByID returns ErrNotFound: not an execution row, fall through to history. + mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return(nil, fmt.Errorf("%w: execution %s", config.ErrNotFound, r.PurchaseID)) mockStore.On("GetPurchaseHistoryByPurchaseID", ctx, r.PurchaseID).Return(r, nil) h := &Handler{config: mockStore, auth: mockAuth} @@ -204,8 +204,8 @@ func TestRevokePurchase_GCPReturns422(t *testing.T) { r := armReservationRecord() r.Provider = "gcp" - // GetExecutionByID returns (nil, nil): not an execution row, fall through to history. - mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return((*config.PurchaseExecution)(nil), nil) + // GetExecutionByID returns ErrNotFound: not an execution row, fall through to history. + mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return(nil, fmt.Errorf("%w: execution %s", config.ErrNotFound, r.PurchaseID)) mockStore.On("GetPurchaseHistoryByPurchaseID", ctx, r.PurchaseID).Return(r, nil) h := &Handler{config: mockStore, auth: mockAuth} @@ -231,8 +231,8 @@ func TestRevokePurchase_AzureOutsideWindow(t *testing.T) { r := armReservationRecord() r.Timestamp = time.Now().UTC().Add(-8 * 24 * time.Hour) // 8 days ago -- window closed - // GetExecutionByID returns (nil, nil): not an execution row, fall through to history. - mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return((*config.PurchaseExecution)(nil), nil) + // GetExecutionByID returns ErrNotFound: not an execution row, fall through to history. + mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return(nil, fmt.Errorf("%w: execution %s", config.ErrNotFound, r.PurchaseID)) mockStore.On("GetPurchaseHistoryByPurchaseID", ctx, r.PurchaseID).Return(r, nil) h := &Handler{config: mockStore, auth: mockAuth} @@ -346,8 +346,8 @@ func TestRevokePurchase_UsesStampedWindow(t *testing.T) { // ...but the stamped window already closed an hour ago. closed := time.Now().UTC().Add(-1 * time.Hour) r.RevocationWindowClosesAt = &closed - // GetExecutionByID returns (nil, nil): not an execution row, fall through to history. - mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return((*config.PurchaseExecution)(nil), nil) + // GetExecutionByID returns ErrNotFound: not an execution row, fall through to history. + mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return(nil, fmt.Errorf("%w: execution %s", config.ErrNotFound, r.PurchaseID)) mockStore.On("GetPurchaseHistoryByPurchaseID", ctx, r.PurchaseID).Return(r, nil) h := &Handler{config: mockStore, auth: mockAuth} @@ -1004,7 +1004,7 @@ func TestLoadAndRevokePurchaseHistory_RevocationInFlightReturns207(t *testing.T) r.RevocationInFlight = true r.RevokedAt = nil - mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return((*config.PurchaseExecution)(nil), nil) + mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return(nil, fmt.Errorf("%w: execution %s", config.ErrNotFound, r.PurchaseID)) mockStore.On("GetPurchaseHistoryByPurchaseID", ctx, r.PurchaseID).Return(r, nil) h := &Handler{config: mockStore, auth: mockAuth} @@ -1044,7 +1044,7 @@ func TestRevokePurchase_AzureWithinSafetyMarginRejected(t *testing.T) { windowCloses := purchasedAt.AddDate(0, 0, AzureRevocationWindowDays) r.RevocationWindowClosesAt = &windowCloses - mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return((*config.PurchaseExecution)(nil), nil) + mockStore.On("GetExecutionByID", ctx, r.PurchaseID).Return(nil, fmt.Errorf("%w: execution %s", config.ErrNotFound, r.PurchaseID)) mockStore.On("GetPurchaseHistoryByPurchaseID", ctx, r.PurchaseID).Return(r, nil) h := &Handler{config: mockStore, auth: mockAuth} diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 115b48cf0..946a456ff 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1771,8 +1771,9 @@ func TestHandler_getPurchaseDetails_NotFound(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - // GetExecutionByID returns (nil, nil) for a missing row (issue #976). - mockStore.On("GetExecutionByID", ctx, "99999999-9999-9999-9999-999999999999").Return(nil, nil) + // GetExecutionByID returns an ErrNotFound-wrapping error for a missing row. + mockStore.On("GetExecutionByID", ctx, "99999999-9999-9999-9999-999999999999"). + Return(nil, fmt.Errorf("%w: execution 99999999-9999-9999-9999-999999999999", config.ErrNotFound)) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1803,7 +1804,8 @@ func TestHandler_getPurchaseDetails_NilExecution(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("GetExecutionByID", ctx, "99999999-9999-9999-9999-999999999999").Return(nil, nil) + mockStore.On("GetExecutionByID", ctx, "99999999-9999-9999-9999-999999999999"). + Return(nil, fmt.Errorf("%w: execution 99999999-9999-9999-9999-999999999999", config.ErrNotFound)) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1825,8 +1827,8 @@ func TestHandler_getPurchaseDetails_NilExecution(t *testing.T) { // TestHandler_getPurchaseDetails_NotFound_IsClientError is a focused // regression for issue #431: a missing execution must produce a 404 ClientError, // never a 500, so that callers cannot infer UUID existence by observing a -// status-code difference. GetExecutionByID returns (nil, nil) for a missing row -// (issue #976); the handler maps that sentinel to a 404. +// status-code difference. GetExecutionByID returns an ErrNotFound-wrapping +// error for a missing row; the handler maps that sentinel to a 404. func TestHandler_getPurchaseDetails_NotFound_IsClientError(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) @@ -1844,8 +1846,9 @@ func TestHandler_getPurchaseDetails_NotFound_IsClientError(t *testing.T) { mockAuth.grantAdmin() const missingID = "dddddddd-dddd-dddd-dddd-dddddddddddd" - // GetExecutionByID returns (nil, nil) for a missing row (issue #976). - mockStore.On("GetExecutionByID", ctx, missingID).Return(nil, nil) + // GetExecutionByID returns an ErrNotFound-wrapping error for a missing row. + mockStore.On("GetExecutionByID", ctx, missingID). + Return(nil, fmt.Errorf("%w: execution %s", config.ErrNotFound, missingID)) handler := &Handler{config: mockStore, auth: mockAuth} req := &events.LambdaFunctionURLRequest{ diff --git a/internal/api/scoping.go b/internal/api/scoping.go index 41edd8936..57280ec6c 100644 --- a/internal/api/scoping.go +++ b/internal/api/scoping.go @@ -2,6 +2,7 @@ package api import ( "context" + "errors" "fmt" "github.com/LeanerCloud/CUDly/internal/auth" @@ -134,12 +135,12 @@ func (h *Handler) requireExecutionAccess(ctx context.Context, session *Session, return nil } execution, err := h.config.GetExecutionByID(ctx, executionID) + if errors.Is(err, config.ErrNotFound) { + return errNotFound + } if err != nil { return fmt.Errorf("failed to get execution: %w", err) } - if execution == nil { - return errNotFound - } return h.requirePlanAccess(ctx, session, execution.PlanID) } diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index b078ad0de..5ebd165a3 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -920,10 +920,11 @@ func (s *PostgresStore) TransitionExecutionStatus(ctx context.Context, execution if len(records) == 0 { existing, existErr := s.GetExecutionByID(ctx, executionID) - if existErr != nil || existing == nil { + if existErr != nil { // Wrap ErrNotFound so callers (e.g. the purchase reaper) can // use errors.Is to distinguish "row vanished mid-flight" — a // legitimate CAS race-loss — from a hard DB error. + // GetExecutionByID returns ErrNotFound when the row is absent. return nil, fmt.Errorf("%w: execution %s", ErrNotFound, executionID) } // Wrap ErrExecutionNotInExpectedStatus so callers can use @@ -992,9 +993,6 @@ func (s *PostgresStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, ex if existErr != nil { return false, "", fmt.Errorf("execution not found or db error: %w", existErr) } - if existing == nil { - return false, "", fmt.Errorf("execution not found: %s", executionID) - } return false, existing.Status, nil } @@ -1052,9 +1050,6 @@ func (s *PostgresStore) CancelScheduledExecutionAtomic(ctx context.Context, tx p if existErr != nil { return false, "", fmt.Errorf("execution not found or db error: %w", existErr) } - if existing == nil { - return false, "", fmt.Errorf("execution not found: %s", executionID) - } return false, existing.Status, nil } @@ -1251,10 +1246,10 @@ func (s *PostgresStore) GetPendingExecutionsTx(ctx context.Context, tx pgx.Tx) ( } // GetExecutionByID retrieves a purchase execution by execution ID. -// Returns (nil, nil) when no row matches executionID so callers can cleanly -// distinguish "not found" (nil execution, nil error) from a real DB failure -// (nil execution, non-nil error). All callers must check the execution for -// nil before use (closes issue #976). +// Returns an error wrapping ErrNotFound when no row matches executionID so +// callers can cleanly distinguish "not found" (errors.Is(err, ErrNotFound)) +// from a real DB failure (any other non-nil error). A nil error guarantees +// a non-nil execution (fail-loud contract; issues #976, #1339). func (s *PostgresStore) GetExecutionByID(ctx context.Context, executionID string) (*PurchaseExecution, error) { query := ` SELECT plan_id, execution_id, status, step_number, scheduled_date, @@ -1275,7 +1270,7 @@ func (s *PostgresStore) GetExecutionByID(ctx context.Context, executionID string } if len(executions) == 0 { - return nil, nil + return nil, fmt.Errorf("%w: execution %s", ErrNotFound, executionID) } return &executions[0], nil diff --git a/internal/config/store_postgres_db_test.go b/internal/config/store_postgres_db_test.go index 868536d2f..9b2e322cb 100644 --- a/internal/config/store_postgres_db_test.go +++ b/internal/config/store_postgres_db_test.go @@ -594,9 +594,10 @@ func TestPostgresStoreDB_PurchaseExecutions(t *testing.T) { t.Run("GetExecutionByID not found", func(t *testing.T) { nonexistentID := uuid.New().String() exec, err := store.GetExecutionByID(ctx, nonexistentID) - // Contract: not-found lookups return (nil, nil); every caller maps - // the nil execution to its own not-found error. - require.NoError(t, err) + // Contract: not-found lookups fail loud with an error wrapping + // ErrNotFound; callers use errors.Is to map it to their own 404. + require.Error(t, err) + require.ErrorIs(t, err, ErrNotFound) assert.Nil(t, exec) }) diff --git a/internal/config/store_postgres_recommendations_test.go b/internal/config/store_postgres_recommendations_test.go index eb0c57f6e..28502f40f 100644 --- a/internal/config/store_postgres_recommendations_test.go +++ b/internal/config/store_postgres_recommendations_test.go @@ -26,6 +26,22 @@ func setupRecommendationsStore(ctx context.Context, t *testing.T) (*config.Postg return store, func() { container.Cleanup(ctx) } } +// seedRecommendationCloudAccount registers a cloud account so that +// recommendations rows referencing it satisfy the FK on +// recommendations.cloud_account_id (migration 000030). Production upserts +// only ever carry registered account IDs, so tests must create the account +// first. +func seedRecommendationCloudAccount(ctx context.Context, t *testing.T, store *config.PostgresStore, id, provider, externalID string) { + t.Helper() + require.NoError(t, store.CreateCloudAccount(ctx, &config.CloudAccount{ + ID: id, + Name: "rec-test-" + externalID, + Provider: provider, + ExternalID: externalID, + Enabled: true, + }), "seeding cloud account %s failed", id) +} + func awsRec(id, service, region, resourceType string, savings float64) config.RecommendationRecord { return config.RecommendationRecord{ ID: id, @@ -268,9 +284,13 @@ func TestPostgresStore_UpsertRecommendations_AccountScopedEviction(t *testing.T) defer cleanup() // Two registered Azure accounts; valid UUIDs because the - // account_key generated column is UUID-typed. + // account_key generated column is UUID-typed. The accounts must exist + // in cloud_accounts: recommendations.cloud_account_id carries an FK + // (migration 000030). acct1 := "11111111-1111-1111-1111-111111111111" acct2 := "22222222-2222-2222-2222-222222222222" + seedRecommendationCloudAccount(ctx, t, store, acct1, "azure", "sub-1111") + seedRecommendationCloudAccount(ctx, t, store, acct2, "azure", "sub-2222") t0 := time.Now().UTC().Truncate(time.Second) @@ -324,7 +344,10 @@ func TestPostgresStore_UpsertRecommendations_AmbientAndRegisteredCoexist(t *test store, cleanup := setupRecommendationsStore(ctx, t) defer cleanup() + // The registered account must exist in cloud_accounts: + // recommendations.cloud_account_id carries an FK (migration 000030). registeredAcctID := "33333333-3333-3333-3333-333333333333" + seedRecommendationCloudAccount(ctx, t, store, registeredAcctID, "aws", "333333333333") t0 := time.Now().UTC().Truncate(time.Second) diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index 826327eae..3c3225e77 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -3,6 +3,7 @@ package purchase import ( "context" "crypto/subtle" + "errors" "fmt" "time" @@ -26,12 +27,12 @@ func (m *Manager) ApproveExecution(ctx context.Context, executionID, token, acto logging.Infof("purchase[%s]: ApproveExecution entry (auth=token actor=%q)", executionID, maskActor(actor)) execution, err := m.config.GetExecutionByID(ctx, executionID) + if errors.Is(err, config.ErrNotFound) { + return fmt.Errorf("execution not found: %s", executionID) + } if err != nil { return fmt.Errorf("failed to get execution: %w", err) } - if execution == nil { - return fmt.Errorf("execution not found: %s", executionID) - } // Validate token using constant-time comparison to prevent timing attacks. if execution.ApprovalToken == "" || token == "" { @@ -242,12 +243,12 @@ func (m *Manager) CancelExecution(ctx context.Context, executionID, token, actor // CancelExecution to keep both functions below the gocyclo threshold. func (m *Manager) loadCancelableExecution(ctx context.Context, executionID, token string) (*config.PurchaseExecution, error) { execution, err := m.config.GetExecutionByID(ctx, executionID) + if errors.Is(err, config.ErrNotFound) { + return nil, fmt.Errorf("execution not found: %s", executionID) + } if err != nil { return nil, fmt.Errorf("failed to get execution: %w", err) } - if execution == nil { - return nil, fmt.Errorf("execution not found: %s", executionID) - } if execution.ApprovalToken == "" || token == "" { return nil, fmt.Errorf("invalid approval token") } diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index 51f6c22a4..0b54bfaf9 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -3,6 +3,7 @@ package purchase import ( "context" "errors" + "fmt" "testing" "time" @@ -181,7 +182,7 @@ func TestManager_ApproveExecution_NotFound(t *testing.T) { ctx := context.Background() manager, store, _ := newApproveManager(t) - store.On("GetExecutionByID", ctx, "exec-123").Return(nil, nil) + store.On("GetExecutionByID", ctx, "exec-123").Return(nil, fmt.Errorf("%w: execution exec-123", config.ErrNotFound)) err := manager.ApproveExecution(ctx, "exec-123", "token", "") assert.Error(t, err) @@ -473,7 +474,7 @@ func TestManager_CancelExecution_NotFound(t *testing.T) { mockStore := new(MockConfigStore) mockEmail := new(MockEmailSender) - mockStore.On("GetExecutionByID", ctx, "exec-123").Return(nil, nil) + mockStore.On("GetExecutionByID", ctx, "exec-123").Return(nil, fmt.Errorf("%w: execution exec-123", config.ErrNotFound)) manager := &Manager{ config: mockStore, diff --git a/internal/purchase/messages.go b/internal/purchase/messages.go index ea16e4ec2..661af009d 100644 --- a/internal/purchase/messages.go +++ b/internal/purchase/messages.go @@ -4,6 +4,7 @@ import ( "context" "crypto/subtle" "encoding/json" + "errors" "fmt" "strings" @@ -84,12 +85,12 @@ func (m *Manager) handleExecutePurchase(ctx context.Context, msg AsyncMessage) e } 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) } - if execution == nil { - return fmt.Errorf("execution not found: %s", msg.ExecutionID) - } logging.Infof("Executing purchase from async message: %s", msg.ExecutionID) @@ -199,12 +200,12 @@ func (m *Manager) verifyAsyncApprovalActor(ctx context.Context, msg *AsyncMessag // 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) } - if execution == nil { - return nil, fmt.Errorf("execution not found: %s", msg.ExecutionID) - } if execution.ApprovalToken == "" || msg.Token == "" { return nil, fmt.Errorf("invalid approval token") } diff --git a/internal/purchase/messages_test.go b/internal/purchase/messages_test.go index 444404b9e..fad85a047 100644 --- a/internal/purchase/messages_test.go +++ b/internal/purchase/messages_test.go @@ -99,7 +99,7 @@ func TestManager_ProcessMessage(t *testing.T) { email: mockEmail, dashboardURL: "https://dashboard.example.com", } - mockStore.On("GetExecutionByID", ctx, "exec-notfound").Return(nil, nil) + 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) From 3435ebc7ccefa59e07b904e262ce8b75df48ddd6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 3 Jul 2026 15:51:54 +0200 Subject: [PATCH 4/5] fix(api): filter ErrTxClosed rollback noise and harden CAS probe error handling Review follow-ups on the errcheck/test-fix stack: - commitmentopts.Save: the deferred rollback logged WARN on every successful Save because pgx Tx.Rollback after Commit returns pgx.ErrTxClosed. Swallow that single sentinel (same filter as config.WithTx) so only real rollback failures are logged. - TransitionExecutionStatus CAS probe: a hard DB error from the zero-row follow-up probe was mapped to ErrNotFound, making a DB outage read as a benign race-loss to the purchase reaper. Only errors.Is(existErr, ErrNotFound) maps to the race-loss sentinel; other probe errors propagate. New pgxmock regression test asserts a hard probe error is not ErrNotFound (verified failing pre-fix). - interfaces.go: document the GetExecutionByID contract on the interface method (wraps ErrNotFound; never returns (nil, nil)). - configure-azure/configure-gcp: reader.ReadString('\n') returns (data, io.EOF) for a final unterminated line, so the new error returns discarded valid piped input (printf "r" | cudly configure-azure). New readTrimmedLine helper tolerates io.EOF when data was read; all interactive reads go through it. --- cmd/configure_azure.go | 45 ++++++++++++------- cmd/configure_gcp.go | 14 +++--- internal/commitmentopts/store_postgres.go | 7 ++- internal/config/interfaces.go | 3 ++ internal/config/store_postgres.go | 8 +++- .../config/store_postgres_pgxmock_test.go | 42 +++++++++++++++++ 6 files changed, 91 insertions(+), 28 deletions(-) diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 8589b2a76..597c4474b 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -4,7 +4,9 @@ import ( "bufio" "context" "encoding/json" + "errors" "fmt" + "io" "log" "os" "os/exec" @@ -30,6 +32,18 @@ func validateAzureUUID(uuid, fieldName string) error { return nil } +// readTrimmedLine reads one line from reader and returns it with surrounding +// whitespace trimmed. io.EOF is tolerated when data was read — a final +// unterminated line from piped input (e.g. `printf "r" | cudly configure-azure`) +// is still valid input. io.EOF with no data, or any other error, is returned. +func readTrimmedLine(reader *bufio.Reader) (string, error) { + input, err := reader.ReadString('\n') + if err != nil && !(errors.Is(err, io.EOF) && input != "") { + return "", err + } + return strings.TrimSpace(input), nil +} + // AzureCredentials holds the Azure Service Principal credentials type AzureCredentials struct { TenantID string `json:"tenant_id"` @@ -216,20 +230,20 @@ func collectAzureCredentials(reader *bufio.Reader) (AzureCredentials, error) { func promptForAzureCredentialFields(reader *bufio.Reader, creds *AzureCredentials) error { if creds.TenantID == "" { fmt.Print("Azure Tenant ID: ") - input, err := reader.ReadString('\n') + input, err := readTrimmedLine(reader) if err != nil { return fmt.Errorf("failed to read tenant ID: %w", err) } - creds.TenantID = strings.TrimSpace(input) + creds.TenantID = input } if creds.ClientID == "" { fmt.Print("Client ID (appId): ") - input, err := reader.ReadString('\n') + input, err := readTrimmedLine(reader) if err != nil { return fmt.Errorf("failed to read client ID: %w", err) } - creds.ClientID = strings.TrimSpace(input) + creds.ClientID = input } if creds.ClientSecret == "" { @@ -244,11 +258,11 @@ func promptForAzureCredentialFields(reader *bufio.Reader, creds *AzureCredential if creds.SubscriptionID == "" { fmt.Print("Subscription ID: ") - input, err := reader.ReadString('\n') + input, err := readTrimmedLine(reader) if err != nil { return fmt.Errorf("failed to read subscription ID: %w", err) } - creds.SubscriptionID = strings.TrimSpace(input) + creds.SubscriptionID = input } return nil @@ -277,11 +291,10 @@ func runAzureSetupCommands(reader *bufio.Reader) error { fmt.Println() fmt.Print("Enter your Subscription ID from above: ") - subscriptionID, err := reader.ReadString('\n') + subscriptionID, err := readTrimmedLine(reader) if err != nil { return fmt.Errorf("failed to read subscription ID: %w", err) } - subscriptionID = strings.TrimSpace(subscriptionID) if subscriptionID == "" { return fmt.Errorf("subscription ID is required") @@ -319,11 +332,11 @@ func createAzureServicePrincipal(reader *bufio.Reader, subscriptionID string) er fmt.Println() fmt.Printf("[R]un, [S]kip? ") - choice, err := reader.ReadString('\n') + choice, err := readTrimmedLine(reader) if err != nil { return fmt.Errorf("failed to read choice: %w", err) } - choice = strings.ToLower(strings.TrimSpace(choice)) + choice = strings.ToLower(choice) if choice == "r" || choice == "run" || choice == "" { fmt.Println() @@ -338,11 +351,11 @@ func createAzureServicePrincipal(reader *bufio.Reader, subscriptionID string) er if err := cmd.Run(); err != nil { fmt.Printf("Command failed: %v\n", err) fmt.Print("Continue anyway? [y/N]: ") - response, readErr := reader.ReadString('\n') + response, readErr := readTrimmedLine(reader) if readErr != nil { return fmt.Errorf("failed to read response: %w", readErr) } - if strings.ToLower(strings.TrimSpace(response)) != "y" { + if strings.ToLower(response) != "y" { return fmt.Errorf("failed to create service principal: %w", err) } } @@ -360,11 +373,11 @@ func promptAndRunExplicitCommand(reader *bufio.Reader, name, displayCmd string, fmt.Println() fmt.Printf("[R]un, [S]kip? ") - choice, err := reader.ReadString('\n') + choice, err := readTrimmedLine(reader) if err != nil { return fmt.Errorf("failed to read choice: %w", err) } - choice = strings.ToLower(strings.TrimSpace(choice)) + choice = strings.ToLower(choice) switch choice { case "r", "run", "": @@ -396,11 +409,11 @@ func executeExplicitCommand(displayCmd string, program string, args ...string) e fmt.Printf("Command failed: %v\n", err) fmt.Print("Continue anyway? [y/N]: ") reader := bufio.NewReader(os.Stdin) - response, readErr := reader.ReadString('\n') + response, readErr := readTrimmedLine(reader) if readErr != nil { return fmt.Errorf("failed to read response: %w", readErr) } - if strings.ToLower(strings.TrimSpace(response)) != "y" { + if strings.ToLower(response) != "y" { return fmt.Errorf("command failed: %w", err) } } diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index e756940ef..d80ef535d 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -178,11 +178,10 @@ func getGCPCredentialsFilePath(reader *bufio.Reader) (string, error) { if credsFile == "" { fmt.Print("Path to GCP service account JSON key file: ") var readErr error - credsFile, readErr = reader.ReadString('\n') + credsFile, readErr = readTrimmedLine(reader) if readErr != nil { return "", fmt.Errorf("failed to read credentials file path: %w", readErr) } - credsFile = strings.TrimSpace(credsFile) } if credsFile == "" { @@ -363,11 +362,10 @@ func runGCPSetupCommands(reader *bufio.Reader) (string, error) { // returns an error if the result is empty. func readRequiredInputLine(reader *bufio.Reader, prompt, fieldName string) (string, error) { fmt.Print(prompt) - value, err := reader.ReadString('\n') + value, err := readTrimmedLine(reader) if err != nil { return "", fmt.Errorf("failed to read %s: %w", fieldName, err) } - value = strings.TrimSpace(value) if value == "" { return "", fmt.Errorf("%s is required", fieldName) } @@ -381,11 +379,11 @@ func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd string, progr fmt.Println() fmt.Printf("[R]un, [S]kip? ") - choice, err := reader.ReadString('\n') + choice, err := readTrimmedLine(reader) if err != nil { return fmt.Errorf("failed to read choice: %w", err) } - choice = strings.ToLower(strings.TrimSpace(choice)) + choice = strings.ToLower(choice) switch choice { case "r", "run", "": @@ -417,11 +415,11 @@ func executeGCPCommand(displayCmd string, program string, args ...string) error fmt.Printf("Command failed: %v\n", err) fmt.Print("Continue anyway? [y/N]: ") reader := bufio.NewReader(os.Stdin) - response, readErr := reader.ReadString('\n') + response, readErr := readTrimmedLine(reader) if readErr != nil { return fmt.Errorf("failed to read response: %w", readErr) } - if strings.ToLower(strings.TrimSpace(response)) != "y" { + if strings.ToLower(response) != "y" { return fmt.Errorf("command failed: %w", err) } } diff --git a/internal/commitmentopts/store_postgres.go b/internal/commitmentopts/store_postgres.go index 99457d0a1..f9fab7b1b 100644 --- a/internal/commitmentopts/store_postgres.go +++ b/internal/commitmentopts/store_postgres.go @@ -2,6 +2,7 @@ package commitmentopts import ( "context" + "errors" "fmt" "github.com/LeanerCloud/CUDly/internal/database" @@ -96,9 +97,11 @@ func (s *PostgresStore) Save(ctx context.Context, combos []Combo, sourceAccountI if err != nil { return fmt.Errorf("begin tx: %w", err) } - // Rollback is a no-op after a successful Commit. + // After a successful Commit, Rollback returns pgx.ErrTxClosed; swallow + // that single sentinel so the happy path stays quiet and only real + // rollback failures on the error path get logged. defer func() { - if rErr := tx.Rollback(ctx); rErr != nil { + if rErr := tx.Rollback(ctx); rErr != nil && !errors.Is(rErr, pgx.ErrTxClosed) { logging.Warnf("commitmentopts.Save: rollback failed: %v", rErr) } }() diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index a6ba6e43d..51b2bf5ef 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -60,6 +60,9 @@ type StoreInterface interface { // The recovery sweep in the purchase manager re-drives these into a // terminal "failed" state so they can never sit permanently approved. GetStaleApprovedExecutions(ctx context.Context, olderThan time.Duration) ([]PurchaseExecution, error) + // GetExecutionByID retrieves a purchase execution by execution ID. + // Returns an error wrapping ErrNotFound when no execution exists; + // never returns (nil, nil). A nil error guarantees a non-nil execution. GetExecutionByID(ctx context.Context, executionID string) (*PurchaseExecution, error) GetExecutionByPlanAndDate(ctx context.Context, planID string, scheduledDate time.Time) (*PurchaseExecution, error) // CountPendingExecutionsForAccount returns the number of purchase_executions diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 5ebd165a3..5317234b4 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -920,13 +920,17 @@ func (s *PostgresStore) TransitionExecutionStatus(ctx context.Context, execution if len(records) == 0 { existing, existErr := s.GetExecutionByID(ctx, executionID) - if existErr != nil { + if errors.Is(existErr, ErrNotFound) { // Wrap ErrNotFound so callers (e.g. the purchase reaper) can // use errors.Is to distinguish "row vanished mid-flight" — a // legitimate CAS race-loss — from a hard DB error. - // GetExecutionByID returns ErrNotFound when the row is absent. return nil, fmt.Errorf("%w: execution %s", ErrNotFound, executionID) } + if existErr != nil { + // A hard DB error during the probe must NOT read as a benign + // race-loss: propagate it so callers see the outage. + return nil, fmt.Errorf("transition %s: probe after zero-row CAS failed: %w", executionID, existErr) + } // Wrap ErrExecutionNotInExpectedStatus so callers can use // errors.Is to recognise CAS rejection (status changed between // SELECT and UPDATE) as race-lost rather than a real error. diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 5cd5447d9..ae157c24a 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -2070,3 +2070,45 @@ func TestPGXMock_ListAccountRegistrations_SearchEscapesBackslash(t *testing.T) { func errNoRows() error { return pgx.ErrNoRows } + +// ─── TransitionExecutionStatus CAS probe ───────────────────────────────────── + +// TestPGXMock_TransitionExecutionStatus_ProbeHardErrorNotMappedToNotFound pins +// the CAS-probe contract: when the UPDATE matches zero rows and the follow-up +// GetExecutionByID probe fails with a hard DB error (outage), the error must +// propagate as-is and NOT be mapped to ErrNotFound, which callers like the +// purchase reaper treat as a benign race-loss. +func TestPGXMock_TransitionExecutionStatus_ProbeHardErrorNotMappedToNotFound(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + execCols := []string{ + "plan_id", "execution_id", "status", "step_number", "scheduled_date", + "notification_sent", "approval_token", "recommendations", + "total_upfront_cost", "estimated_savings", "completed_at", "error", "expires_at", + "cloud_account_id", "source", "approved_by", "cancelled_by", "capacity_percent", + "created_by_user_id", "retry_execution_id", "retry_attempt_n", + "approval_token_expires_at", + "executed_by_user_id", "executed_at", "pre_approval_skip_reason", + "idempotency_key", "scheduled_execution_at", + } + + // CAS UPDATE matches zero rows (status already transitioned or row gone). + mock.ExpectQuery(`UPDATE purchase_executions`). + WithArgs(anyArgsCfg(4)...). + WillReturnRows(pgxmock.NewRows(execCols)) + + // Probe fails with a hard DB error, not an empty result. + dbErr := errors.New("connection refused") + mock.ExpectQuery(`SELECT plan_id, execution_id, status`). + WithArgs("exec-probe-err"). + WillReturnError(dbErr) + + _, err := store.TransitionExecutionStatus(ctx, "exec-probe-err", []string{"pending"}, "approved", nil) + require.Error(t, err) + assert.False(t, errors.Is(err, ErrNotFound), "hard probe error must not be mapped to ErrNotFound") + assert.False(t, errors.Is(err, ErrExecutionNotInExpectedStatus), "hard probe error must not read as CAS rejection") + assert.ErrorIs(t, err, dbErr) + assert.NoError(t, mock.ExpectationsWereMet()) +} From 36d383813d260caea7d37e57df2da319fe5616e9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 3 Jul 2026 16:15:19 +0200 Subject: [PATCH 5/5] fix(cli): thread stdin reader through prompts and fail loud on credential load errors CodeRabbit review follow-ups on PR #1343: - configure-azure/configure-gcp: executeExplicitCommand and executeGCPCommand created a fresh bufio.NewReader(os.Stdin) for the "Continue anyway?" retry prompt, which can drop input already buffered by the caller's reader and break piped input after earlier prompts. The caller's reader is now threaded through so all prompts consume one consistent buffered stream. - credentials resolver: a LoadRaw failure while peeking at the stored WIF config was logged and treated as absence, silently flipping a stored-JSON workload-identity-federation account onto the federated path on a store outage (auth behavior change on failure). LoadRaw returns (nil, nil) only for genuine absence, so store errors now propagate. New regression test asserts the error surfaces instead of falling through (verified failing pre-fix). --- cmd/configure_azure.go | 11 +++-- cmd/configure_gcp.go | 11 +++-- internal/credentials/resolver.go | 8 ++-- internal/credentials/resolver_extra_test.go | 45 +++++++++++++++++++++ 4 files changed, 64 insertions(+), 11 deletions(-) diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 597c4474b..7449ea7e0 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -381,7 +381,7 @@ func promptAndRunExplicitCommand(reader *bufio.Reader, name, displayCmd string, switch choice { case "r", "run", "": - return executeExplicitCommand(displayCmd, program, args...) + return executeExplicitCommand(reader, displayCmd, program, args...) case "s", "skip": fmt.Printf("Skipping %s\n", name) return nil @@ -391,8 +391,12 @@ func promptAndRunExplicitCommand(reader *bufio.Reader, name, displayCmd string, } } -// executeExplicitCommand runs a command with explicit program and arguments -func executeExplicitCommand(displayCmd string, program string, args ...string) error { +// executeExplicitCommand runs a command with explicit program and arguments. +// The caller's reader is threaded through to the retry prompt so all input +// is consumed from one consistent buffered stream (a fresh +// bufio.NewReader(os.Stdin) here would drop input already buffered by the +// caller's reader, breaking piped input after earlier prompts). +func executeExplicitCommand(reader *bufio.Reader, displayCmd string, program string, args ...string) error { fmt.Println() fmt.Printf("Executing: %s\n", displayCmd) fmt.Println(strings.Repeat("-", 60)) @@ -408,7 +412,6 @@ func executeExplicitCommand(displayCmd string, program string, args ...string) e if err != nil { fmt.Printf("Command failed: %v\n", err) fmt.Print("Continue anyway? [y/N]: ") - reader := bufio.NewReader(os.Stdin) response, readErr := readTrimmedLine(reader) if readErr != nil { return fmt.Errorf("failed to read response: %w", readErr) diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index d80ef535d..1cd4ca010 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -387,7 +387,7 @@ func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd string, progr switch choice { case "r", "run", "": - return executeGCPCommand(displayCmd, program, args...) + return executeGCPCommand(reader, displayCmd, program, args...) case "s", "skip": fmt.Printf("Skipping %s\n", name) return nil @@ -397,8 +397,12 @@ func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd string, progr } } -// executeGCPCommand runs a gcloud command with explicit program and arguments -func executeGCPCommand(displayCmd string, program string, args ...string) error { +// executeGCPCommand runs a gcloud command with explicit program and arguments. +// The caller's reader is threaded through to the retry prompt so all input +// is consumed from one consistent buffered stream (a fresh +// bufio.NewReader(os.Stdin) here would drop input already buffered by the +// caller's reader, breaking piped input after earlier prompts). +func executeGCPCommand(reader *bufio.Reader, displayCmd string, program string, args ...string) error { fmt.Println() fmt.Printf("Executing: %s\n", displayCmd) fmt.Println(strings.Repeat("-", 60)) @@ -414,7 +418,6 @@ func executeGCPCommand(displayCmd string, program string, args ...string) error if err != nil { fmt.Printf("Command failed: %v\n", err) fmt.Print("Continue anyway? [y/N]: ") - reader := bufio.NewReader(os.Stdin) response, readErr := readTrimmedLine(reader) if readErr != nil { return fmt.Errorf("failed to read response: %w", readErr) diff --git a/internal/credentials/resolver.go b/internal/credentials/resolver.go index c7af8126f..28555afc6 100644 --- a/internal/credentials/resolver.go +++ b/internal/credentials/resolver.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "fmt" - "log" "os" "path/filepath" "strings" @@ -406,13 +405,16 @@ func resolveGCPWIFCredential( opts GCPResolveOptions, ) (oauth2.TokenSource, error) { // Peek at the stored WIF JSON first — absent means this is a - // federated (secret-free) account. + // federated (secret-free) account. LoadRaw returns (nil, nil) only for + // genuine absence; a store failure must fail loud rather than silently + // flipping a stored-JSON account onto the federated path (auth + // behavior must not change on a store outage). var raw []byte if store != nil { var loadErr error raw, loadErr = store.LoadRaw(ctx, account.ID, CredTypeGCPWIFConfig) if loadErr != nil { - log.Printf("credentials: LoadRaw for account %s: %v (treating as absent)", account.ID, loadErr) + return nil, fmt.Errorf("credentials: LoadRaw WIF config for account %s: %w", account.ID, loadErr) } } diff --git a/internal/credentials/resolver_extra_test.go b/internal/credentials/resolver_extra_test.go index 195094ab3..d31959406 100644 --- a/internal/credentials/resolver_extra_test.go +++ b/internal/credentials/resolver_extra_test.go @@ -11,10 +11,28 @@ import ( "testing" "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/internal/oidc" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// stubOIDCSigner is a minimal oidc.Signer whose methods must never be +// reached in the tests that use it — it only exists so GCPResolveOptions +// looks federated-capable. +type stubOIDCSigner struct{} + +func (stubOIDCSigner) Sign(context.Context, []byte) ([]byte, error) { + return nil, assert.AnError +} +func (stubOIDCSigner) PublicKey(context.Context) (*rsa.PublicKey, error) { + return nil, assert.AnError +} +func (stubOIDCSigner) KeyID(context.Context) (string, error) { + return "", assert.AnError +} + +var _ oidc.Signer = stubOIDCSigner{} + // --------------------------------------------------------------------------- // resolveBastionProvider // --------------------------------------------------------------------------- @@ -261,6 +279,33 @@ func TestResolveGCPTokenSource_WIF_NoStoredCredentials(t *testing.T) { assert.Contains(t, err.Error(), "no gcp credentials stored") } +// TestResolveGCPTokenSource_WIF_StoreErrorFailsLoud pins the fail-loud +// contract on the WIF peek: a LoadRaw failure must propagate, NOT be +// treated as "no stored credential" — otherwise a store outage would +// silently flip a stored-JSON WIF account onto the federated path (the +// options here are deliberately federated-capable so the pre-fix code +// would have taken that path and returned no error). +func TestResolveGCPTokenSource_WIF_StoreErrorFailsLoud(t *testing.T) { + store := newMockStore() + store.err = assert.AnError + + account := &config.CloudAccount{ + ID: "acct1", + GCPAuthMode: "workload_identity_federation", + GCPWIFAudience: "//iam.googleapis.com/projects/1/locations/global/workloadIdentityPools/p/providers/x", + GCPClientEmail: "sa@proj.iam.gserviceaccount.com", + } + opts := GCPResolveOptions{ + Signer: stubOIDCSigner{}, + IssuerURL: "https://cudly.example.com/oidc", + } + + _, err := ResolveGCPTokenSourceWithOpts(context.Background(), account, store, opts) + require.Error(t, err, "store failure must fail loud, not fall through to the federated path") + assert.ErrorIs(t, err, assert.AnError) + assert.Contains(t, err.Error(), "LoadRaw WIF config") +} + func TestResolveGCPTokenSource_InvalidJSON(t *testing.T) { store := newMockStore() store.data["acct1/gcp_service_account"] = []byte("not valid json")