From c3777f90c72055a211fb39b3437b2d3c97cf6963 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 05:04:10 +0100 Subject: [PATCH 01/27] chore(deps): bump cloud-commitments-go modules to 61111428 Picks up the Azure Advisor reservation pricing (go#663), the typed partial-sweep skip accessors and the currency-not-accepted sentinel. Co-Authored-By: Claude Opus 5.5 --- go.mod | 24 ++++++++++++------------ go.sum | 59 +++++++++++++++++++++++++++++----------------------------- 2 files changed, 41 insertions(+), 42 deletions(-) diff --git a/go.mod b/go.mod index a260648d2..a98b657df 100644 --- a/go.mod +++ b/go.mod @@ -34,9 +34,9 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.15.0 github.com/Azure/azure-sdk-for-go/sdk/internal v1.13.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/advisor/armadvisor v1.2.0 // indirect - github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.4.0 // indirect - github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption v1.1.0 // indirect - github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.0.0 // indirect + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0 // indirect + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption v1.2.0 // indirect + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.3.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions v1.3.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql v1.2.0 // indirect github.com/AzureAD/microsoft-authentication-library-for-go v1.10.1 // indirect @@ -88,10 +88,10 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations v1.1.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys v1.5.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.5.0 - github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261026123645-88b95b64b043 - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261026123645-88b95b64b043 - github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261026123645-88b95b64b043 - github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261026123645-88b95b64b043 + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261103222717-61111428a353 + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261103222717-61111428a353 + github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261103222717-61111428a353 + github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261103222717-61111428a353 github.com/aws/aws-lambda-go v1.55.1 github.com/aws/aws-sdk-go-v2/service/kms v1.62.1 github.com/aws/aws-sdk-go-v2/service/lambda v1.113.4 @@ -113,20 +113,19 @@ require ( require ( cel.dev/expr v0.25.3 // indirect - cloud.google.com/go/auth v0.24.1-0.20261001053825-dbc26066f70a // indirect + cloud.google.com/go/auth v0.24.1 // indirect cloud.google.com/go/auth/oauth2adapt v0.3.0 // indirect cloud.google.com/go/monitoring v1.30.0 // indirect cloud.google.com/go/redis v1.24.0 // indirect - cloud.google.com/go/storage v1.62.3 // indirect - cloud.google.com/go/trace v1.11.7 // indirect + cloud.google.com/go/storage v1.70.0 // indirect dario.cat/mergo v1.0.2 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cosmos/armcosmos/v2 v2.7.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/search/armsearch v1.4.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/internal v1.2.0 // indirect github.com/Azure/go-ansiterm v0.0.0-20250102033503-faa5f7b0171c // indirect github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.35.0 // indirect - github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.55.0 // indirect - github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.55.0 // indirect + github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.57.0 // indirect + github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.57.0 // indirect github.com/Microsoft/go-winio v0.6.2 // indirect github.com/agext/levenshtein v1.2.1 // indirect github.com/apparentlymart/go-textseg/v15 v15.0.0 // indirect @@ -154,6 +153,7 @@ require ( github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect github.com/jackc/puddle/v2 v2.2.2 // indirect github.com/klauspost/compress v1.18.7 // indirect + github.com/kr/text v0.2.0 // indirect github.com/lufia/plan9stats v0.0.0-20260330125221-c963978e514e // indirect github.com/magiconair/properties v1.8.10 // indirect github.com/mitchellh/go-wordwrap v1.0.1 // indirect diff --git a/go.sum b/go.sum index 9db63995f..20ad857f5 100644 --- a/go.sum +++ b/go.sum @@ -2,8 +2,8 @@ cel.dev/expr v0.25.3 h1:A2jO8jwOugrrovveCWfj0KEZOfqiLgAcwjpHPhzIGw0= cel.dev/expr v0.25.3/go.mod h1:hrXvqGP6G6gyx8UAHSHJ5RGk//1Oj5nXQ2NI02Nrsg4= cloud.google.com/go v0.123.0 h1:2NAUJwPR47q+E35uaJeYoNhuNEM9kM8SjgRgdeOJUSE= cloud.google.com/go v0.123.0/go.mod h1:xBoMV08QcqUGuPW65Qfm1o9Y4zKZBpGS+7bImXLTAZU= -cloud.google.com/go/auth v0.24.1-0.20261001053825-dbc26066f70a h1:jW1ZLFy9oa5ai8LOI40z6s+DL1cO/L1KSm9hVGnibn8= -cloud.google.com/go/auth v0.24.1-0.20261001053825-dbc26066f70a/go.mod h1:1dmBXa9xfBvBr39ZnDUDkaaA91ccq9CcqENDgJA4d7c= +cloud.google.com/go/auth v0.24.1 h1:NTmd2iJb4DsGIav0KiqFMYI88nMshA2wnFMBatRmoCs= +cloud.google.com/go/auth v0.24.1/go.mod h1:1dmBXa9xfBvBr39ZnDUDkaaA91ccq9CcqENDgJA4d7c= cloud.google.com/go/auth/oauth2adapt v0.3.0 h1:FY8oSZpCYoUNv6QxVODuMjQz4IlSOVeiQtZ08vLPz88= cloud.google.com/go/auth/oauth2adapt v0.3.0/go.mod h1:7+2uCm7++XFO+/lN06c2HXpDXb/NMNn2/UwyBPbTnkk= cloud.google.com/go/compute v1.74.0 h1:GxtfiSN4hoA/938OAruNaZ746F6rUtZNyDQvXcXutrA= @@ -28,10 +28,10 @@ cloud.google.com/go/resourcemanager v1.16.0 h1:aDf2RuuQ1Z+kbHHDzHyJNMHPGRFK+cMYf cloud.google.com/go/resourcemanager v1.16.0/go.mod h1:Hn4HPkLRnTuiUhFEFJg736Brt7BwlS84xYU06sc3STc= cloud.google.com/go/secretmanager v1.23.0 h1:tUmPWfKapcrCsLXdJQIokbJamK5leTGAyxVhPXG1x90= cloud.google.com/go/secretmanager v1.23.0/go.mod h1:xDP/NNLKgQGGKcOenNnflIPW49MBj5w1EcakXjRLZHg= -cloud.google.com/go/storage v1.62.3 h1:SZq1t23NCI+e96dH77Dg3PEfsNNEjqO8zE5AnD8gVD0= -cloud.google.com/go/storage v1.62.3/go.mod h1:cpYz/kRVZ+UQAF1uHeea10/9ewcRbxGoGNKsS9daSXA= -cloud.google.com/go/trace v1.11.7 h1:kDNDX8JkaAG3R2nq1lIdkb7FCSi1rCmsEtKVsty7p+U= -cloud.google.com/go/trace v1.11.7/go.mod h1:TNn9d5V3fQVf6s4SCveVMIBS2LJUqo73GACmq/Tky0s= +cloud.google.com/go/storage v1.70.0 h1:LQNHn0rRF94jVFfF3e416IzdQqIufa5GfF6HilQxb80= +cloud.google.com/go/storage v1.70.0/go.mod h1:W84cNa070TZIegvTW67CBY+f5rxux8EJMb7YyiHdrls= +cloud.google.com/go/trace v1.16.0 h1:GmQovzFc5F0CNfl0VLgL64aoTtu7xsM0YajW2GlG9+E= +cloud.google.com/go/trace v1.16.0/go.mod h1:r+bdAn16dKLSV1G2D5v3e58IlQlizfxWrUfjx7kM7X0= dario.cat/mergo v1.0.2 h1:85+piFYR1tMbRrLcDwR18y4UKJ3aH1Tbzi24VRW1TK8= dario.cat/mergo v1.0.2/go.mod h1:E/hbnu0NxMFBjpMIE34DRGLWqDy0g5FuKDhCb31ngxA= github.com/AdaLogics/go-fuzz-headers v0.0.0-20240806141605-e8a1dd7889d6 h1:He8afgbRMd7mFxO99hRNu+6tazq8nFF9lIwo9JFroBk= @@ -48,20 +48,18 @@ github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/advisor/armadvisor v1.2.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/advisor/armadvisor v1.2.0/go.mod h1:oZ73p8dR7aZI+TJo5Ul92oCoVubMYPBo39eTsWa0AiQ= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits v1.0.0 h1:4JHm1qT4VKYZ8aGP1dJBg+wk/WgN/KitLNtQs6jFNfA= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits v1.0.0/go.mod h1:GBFVpPKOTooUG7VM93ip046AqLf4qubcIEsU3js4v3g= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.4.0 h1:QfV5XZt6iNa2aWMAt96CZEbfJ7kgG/qYIpq465Shr5E= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.4.0/go.mod h1:uYt4CfhkJA9o0FN7jfE5minm/i4nUE4MjGUJkzB6Zs8= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption v1.1.0 h1:pTIng5JZfGKPA4WT8QjEPGOD5KK2CoCBkecWgtq3Cuc= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption v1.1.0/go.mod h1:0vCBR1wgGwZeGmloJ+eCWIZF2S47grTXRzj2mftg2Nk= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0 h1:LkHbJbgF3YyvC53aqYGR+wWQDn2Rdp9AQdGndf9QvY4= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0/go.mod h1:QyiQdW4f4/BIfB8ZutZ2s+28RAgfa/pT+zS++ZHyM1I= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption v1.2.0 h1:TAbicMLAaCP73UAoRwAoVh0DVuyzdWT/psQr4pG1vHY= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption v1.2.0/go.mod h1:a1Pzix6xp1+Y9/hzJUAsx81QcUOHWMLgbcRtYTbdFuw= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cosmos/armcosmos/v2 v2.7.0 h1:mTrlTrd4rdq32sUpDZhKJw8pfHaAqaEhZTuGH4WMfDQ= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cosmos/armcosmos/v2 v2.7.0/go.mod h1:M7VOO9cI4UMIkZGo+a5RS9HcsQeQPRQ104Py9Vug3KU= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal v1.1.2 h1:mLY+pNLjCUeKhgnAJWAKhEUQM+RJQo2H1fuGSw1Ky1E= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal v1.1.2/go.mod h1:FbdwsQ2EzwvXxOPcMFYO8ogEc9uMMIj3YkmCdXdAFmk= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v2 v2.0.0 h1:PTFGRSlMKCQelWwxUyYVEUqseBJVemLyqWJjvMyt0do= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v2 v2.0.0/go.mod h1:LRr2FzBTQlONPPa5HREE5+RjSCTXl7BwOvYOaWTqCaI= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v3 v3.1.0 h1:2qsIIvxVT+uE6yrNldntJKlLRgxGbZ85kgtz5SNBhMw= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v3 v3.1.0/go.mod h1:AW8VEadnhw9xox+VaVd9sP7NjzOAnaZBLRH6Tq3cJ38= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.0.0 h1:zp+znRAHKLSewbw+WWKIMgCaFNxEXt9AwjxmW5fCnck= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.0.0/go.mod h1:nEvLUni7GO5ukfEYtmrUfz08Puqd2FP9d8sCZazm5W4= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.3.0 h1:EkL5dmUoy1OlzVfsbkcHayOvOJgheyRYL3wM/RHizzg= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.3.0/go.mod h1:DiazWkJHUUKUZGpIdV7JhDTjebBxdfsJ386dE5w7G3o= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations v1.1.0 h1:0OO/3K+SKt45gXiOU4gHRILOLeNOUZdqeNO47Mq6iN8= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations v1.1.0/go.mod h1:2FDnHkGwh1BFK1ZQt+iDJU47Cg9Y28F0W3FSI389NOA= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0 h1:Dd+RhdJn0OTtVGaeDLZpcumkIVCtA/3/Fo42+eoYvVM= @@ -86,20 +84,20 @@ github.com/AzureAD/microsoft-authentication-library-for-go v1.10.1 h1:0O6j18nQDo github.com/AzureAD/microsoft-authentication-library-for-go v1.10.1/go.mod h1:xdYAf5bjkOpsd3auA8riiJW4vneBubt9caavL7626jQ= github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.35.0 h1:bN1gA3of5bXtbnLsRPrwfmbbe7A5UWFlcTHseujLnpc= github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.35.0/go.mod h1:Yj5vHEz/aAepZGliRJsA6uvHAVAQyEwajq9ORCHPxzM= -github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.55.0 h1:UnDZ/zFfG1JhH/DqxIZYU/1CUAlTUScoXD/LcM2Ykk8= -github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.55.0/go.mod h1:IA1C1U7jO/ENqm/vhi7V9YYpBsp+IMyqNrEN94N7tVc= -github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.55.0 h1:7t/qx5Ost0s0wbA/VDrByOooURhp+ikYwv20i9Y07TQ= -github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.55.0/go.mod h1:vB2GH9GAYYJTO3mEn8oYwzEdhlayZIdQz6zdzgUIRvA= -github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.55.0 h1:0s6TxfCu2KHkkZPnBfsQ2y5qia0jl3MMrmBhu3nCOYk= -github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.55.0/go.mod h1:Mf6O40IAyB9zR/1J8nGDDPirZQQPbYJni8Yisy7NTMc= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261026123645-88b95b64b043 h1:aNxMUAEefkT973eGTUbVppd1X99zLKWRUYmJWfO1BUI= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261026123645-88b95b64b043/go.mod h1:rEJErEsL40Np/ttf0RDoNNC2XQsN1GmWyFf34y0JLD8= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261026123645-88b95b64b043 h1:UjP0mxD+iv/ZIZg6hIm6dWHILzNX+0T/WCgiDUk5c+8= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261026123645-88b95b64b043/go.mod h1:yVHEL5ceNS/CsDatzLH0+Qmwtoi7yY4tIHvqyvhdtig= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261026123645-88b95b64b043 h1:uhHCm5B8G6sGGoRCOyUp41tbza4W3pkh2EeUwT8UVKo= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261026123645-88b95b64b043/go.mod h1:zA51HKXtBvHHMYd+owRDmn4p8ialZrLFzI9I052uUmo= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261026123645-88b95b64b043 h1:ryqPNyihinNu9065Ysi4c9sWP90yX9jeBP7fDOrO6jw= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261026123645-88b95b64b043/go.mod h1:/HxBqXBb5v69+EHLsibHnekkEYRP8TvyCmT6d9QRxt0= +github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.57.0 h1:jLdiS1vO+XJFyDSWRHBx56r4s/NNtcl5J6KyCcWUX/w= +github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.57.0/go.mod h1:8lmpHY+1VRoteiOwyrQMDt1YGXOrFKCz+1wJW7n3ODY= +github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.57.0 h1:cSjUzZ7KU8hicTgzaSv9NmSyM9fTVK3y5lsBUl3wOis= +github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.57.0/go.mod h1:dzcEjy1WJ0Q4u9twNR3LcLhNoYMRCrMCMafpxa0TjPQ= +github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.57.0 h1:RoO5+d7uCmDqovLrHCr2/BuViUXvdcrNxyNM1pN9dDQ= +github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.57.0/go.mod h1:YqwkQPrWSC7+byyc1VlKbWLBF5JsW5IoL6xUkemYSXk= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261103222717-61111428a353 h1:PpC722KByQTXdsdUIXuBzRXpklEu3+SLKfMipfPOtPg= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261103222717-61111428a353/go.mod h1:pseCFRgm1rzBKomktjw6E0ayxmVE7XfHHiOmew4ka+g= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261103222717-61111428a353 h1:91krJUF0e9AC0Y0mc6nAVi4MsMtGiZVDlSgbZ/VT+9E= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261103222717-61111428a353/go.mod h1:NjNxaaNeYU3CTAcWUCOxaN+NeY7aveSyI/jNXEKz7/c= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261103222717-61111428a353 h1:jCiXQIXO42bkvQPEaZ0IIQBzxGz3BPptB74cIumdiCo= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261103222717-61111428a353/go.mod h1:X+73EdStvYV6pqEVZZFyMfLG1frORlO9B1IsuK0LfOs= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261103222717-61111428a353 h1:faFzHnGyaYgdN3rU0Ukkq6hHXjpOEcaX6W82Mz+DfnM= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261103222717-61111428a353/go.mod h1:aj4d0YEmBg1A46rIXKG+GkTFc1d/xwmveJ97kDD1oPs= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU= github.com/agext/levenshtein v1.2.1 h1:QmvMAjj2aEICytGiWzmxoE0x2KZvE0fvmqMOfy2tjT8= @@ -186,6 +184,7 @@ github.com/coreos/go-oidc/v3 v3.21.0 h1:wZo4Q9Pum8dYEj0eMUPrqR+kvuGkeUplbLpNCkBq github.com/coreos/go-oidc/v3 v3.21.0/go.mod h1:DYCf24+ncYi+XkIH97GY1+dqoRlbaSI26KVTCI9SrY4= github.com/cpuguy83/dockercfg v0.3.2 h1:DlJTyZGBDlXqUZ2Dk2Q3xHs/FtnooJJVaad2S9GKorA= github.com/cpuguy83/dockercfg v0.3.2/go.mod h1:sugsbF4//dDlL/i+S+rtpIWp+5h0BHJHfjj5/jFyUJc= +github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= @@ -349,8 +348,8 @@ go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.69.0 h1:8tvICD4 go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.69.0/go.mod h1:z9+yiacE0IHRqM4qFfkbt/JYlmYXgss8GY/jXoNuPJI= go.opentelemetry.io/otel v1.47.0 h1:j7ALJ/zgkS7Z6aeJW09p8VC9804bC+PpeTfCD4XPnOM= go.opentelemetry.io/otel v1.47.0/go.mod h1:8wS9O2qfXrYrzp6hIF/HOYJJf/wIhFPhR2xLuP+iXQU= -go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.43.0 h1:TC+BewnDpeiAmcscXbGMfxkO+mwYUwE/VySwvw88PfA= -go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.43.0/go.mod h1:J/ZyF4vfPwsSr9xJSPyQ4LqtcTPULFR64KwTikGLe+A= +go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.45.0 h1:dm9iyzn6tioYZtwqaiBSU0TSI8Yu/8dTIbfG0+B49DY= +go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.45.0/go.mod h1:xAvxYjYK28qvt+yu4BYZ/zMmAjwMXINXD6JiMyeB8iI= go.opentelemetry.io/otel/log v1.47.0 h1:cOTS1CcLbSQeZKanGJ+0JpF/+t4PELi3O3bbl2lqCcI= go.opentelemetry.io/otel/log v1.47.0/go.mod h1:9byitSQ5pLC6PpqwGXjqdMKya6ZTswHRZh2vvXT33nw= go.opentelemetry.io/otel/metric v1.47.0 h1:4PptaldXx3Eat1XjMZ68pPJEs5wrhlemctZE9a3UdWY= From 2c29a1e3013c4c4525c8a99e2cbe381046fbe2aa Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 05:05:15 +0100 Subject: [PATCH 02/27] refactor(scheduler): read skipped-recommendation counts from the typed accessors Replace the message regex with the provider error's SkippedCount() and SkipReasons() methods, so the count no longer depends on message text. Co-Authored-By: Claude Opus 5.5 --- internal/scheduler/partial_skip_test.go | 19 ++++++++++++------- internal/scheduler/partial_sweep.go | 20 ++++++-------------- 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/internal/scheduler/partial_skip_test.go b/internal/scheduler/partial_skip_test.go index ad947df78..36abecc6f 100644 --- a/internal/scheduler/partial_skip_test.go +++ b/internal/scheduler/partial_skip_test.go @@ -16,10 +16,14 @@ import ( type skipErr struct { msg string partial bool + count int + reasons []string } -func (e *skipErr) Error() string { return e.msg } -func (e *skipErr) Partial() bool { return e.partial } +func (e *skipErr) Error() string { return e.msg } +func (e *skipErr) Partial() bool { return e.partial } +func (e *skipErr) SkippedCount() int { return e.count } +func (e *skipErr) SkipReasons() []string { return e.reasons } const skipMsg = "gcp recommendations: 3 recommendation(s) skipped: computeengine us-east1: non-USD; cloudsql us-east1: bad plan" @@ -29,7 +33,7 @@ func gcpAmbient(s *Scheduler) ([]config.RecommendationRecord, []string, *sweepGa func TestGCPPartialResult_KeepsRecsAndRecordsSkips(t *testing.T) { t.Setenv("GCP_PROJECT_ID", "proj-a") - s, _ := newPartialSweepScheduler(t, "gcp", fmt.Errorf("collect: %w", &skipErr{msg: skipMsg, partial: true})) + s, _ := newPartialSweepScheduler(t, "gcp", fmt.Errorf("collect: %w", &skipErr{msg: skipMsg, partial: true, count: 3})) recs, roster, gap, err := gcpAmbient(s) @@ -65,7 +69,7 @@ func TestGCPPartialResult_NonPartialErrorsStayHard(t *testing.T) { func TestGCPPartialResult_FanOutKeepsAccountEvictable(t *testing.T) { accounts := []config.CloudAccount{{ID: "a1", Name: "a1", ExternalID: "p1"}, {ID: "a2", Name: "a2", ExternalID: "p2"}} fn := func(_ context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { - gap, err := tolerateIncompleteSweep("gcp", &skipErr{msg: skipMsg, partial: true}) + gap, err := tolerateIncompleteSweep("gcp", &skipErr{msg: skipMsg, partial: true, count: 3}) if acct.ID == "a2" { // A lost project in the same account hold back eviction as before. gap = mergeSweepGaps(gap, gapFromPartialSubscriptions(partialSweepErr("p-x"))) @@ -83,9 +87,10 @@ func TestGCPPartialResult_FanOutKeepsAccountEvictable(t *testing.T) { assert.Equal(t, 1, outcome.Gap.incompleteAccounts) } -func TestGCPPartialResult_UnparsableCountStillRecorded(t *testing.T) { - gap, err := tolerateIncompleteSweep("gcp", &skipErr{msg: "something skipped", partial: true}) +// The count comes from SkippedCount(), never from the message text. +func TestGCPPartialResult_CountComesFromTypedMethod(t *testing.T) { + gap, err := tolerateIncompleteSweep("gcp", &skipErr{msg: "something skipped", partial: true, count: 5}) require.NoError(t, err) require.NotNil(t, gap) - assert.Zero(t, gap.skippedRecommendations) + assert.Equal(t, 5, gap.skippedRecommendations) } diff --git a/internal/scheduler/partial_sweep.go b/internal/scheduler/partial_sweep.go index 43cfcfd2b..fc0679206 100644 --- a/internal/scheduler/partial_sweep.go +++ b/internal/scheduler/partial_sweep.go @@ -4,8 +4,6 @@ import ( "context" "errors" "net/http" - "regexp" - "strconv" "strings" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -52,12 +50,14 @@ func (g *sweepGap) holdsEviction() bool { return g != nil && !g.skippedOnly } -var skippedCountRE = regexp.MustCompile(`(\d+) recommendation\(s\) skipped`) - // partialResult is implemented by provider errors that accompany usable // results (GCP's skipped recommendations). Matching the method, not the // provider's internal type, keeps this provider-agnostic. -type partialResult interface{ Partial() bool } +type partialResult interface { + Partial() bool + SkippedCount() int + SkipReasons() []string +} // tolerateSkippedRecommendations keeps the recommendations returned alongside // a Partial() error and records how many were skipped. Any other error is @@ -68,15 +68,7 @@ func tolerateSkippedRecommendations(providerName string, err error) (*sweepGap, return nil, err } logging.Warnf("%s recommendations partial, keeping the rest: %v", providerName, err) - // The count is parsed from the message because the contract exposes only - // Partial(); an unparsable message still yields a gap, with count 0. - n := 0 - if m := skippedCountRE.FindStringSubmatch(err.Error()); m != nil { - if v, convErr := strconv.Atoi(m[1]); convErr == nil { - n = v - } - } - return &sweepGap{skippedRecommendations: n, skippedOnly: true, reasons: map[string]int{}}, nil + return &sweepGap{skippedRecommendations: p.SkippedCount(), skippedOnly: true, reasons: map[string]int{}}, nil } // gapFromAWSIncomplete summarizes an AWS IncompleteRecommendationsError. From 1135ee73c00751c3d7e6405799acda64cc1af9fa Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 05:06:01 +0100 Subject: [PATCH 03/27] fix(api): return 422 with the library message for a refused exchange currency Azure CalculateExchange wraps common.ErrCurrencyNotAccepted when the net payable currency is missing or not USD. Treat it like the other exchange refusals: 422 carrying the message and a failed audit row, not a generic 500. Co-Authored-By: Claude Opus 5.5 --- internal/api/ri_exchange_refusals.go | 9 +++++++-- internal/api/ri_exchange_refusals_test.go | 6 +++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/internal/api/ri_exchange_refusals.go b/internal/api/ri_exchange_refusals.go index 537802c17..364c10658 100644 --- a/internal/api/ri_exchange_refusals.go +++ b/internal/api/ri_exchange_refusals.go @@ -3,6 +3,7 @@ package api import ( "errors" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" ) @@ -17,12 +18,16 @@ import ( // aliased in provider_wiring.go): a source reservation's applied scope // cannot be carried onto the purchased reservation, or the sources mix // scopes. Returned by CalculateExchange before any Azure call. +// - common.ErrCurrencyNotAccepted: the billing currency a provider +// reported is missing or not USD (Azure CalculateExchange, wrapped with +// %w), so no session is returned and nothing can be executed. // -// Both describe the request or the account it targets, not a server fault, -// and both prove the exchange did not happen. +// These describe the request or the account it targets, not a server fault, +// and all prove the exchange did not happen. var exchangeRefusals = []error{ exchange.ErrQuoteNotUSD, errAzureUnsupportedAppliedScope, + common.ErrCurrencyNotAccepted, } // exchangeRefusalError returns a 422 carrying err's message when err wraps diff --git a/internal/api/ri_exchange_refusals_test.go b/internal/api/ri_exchange_refusals_test.go index 14f5d1949..c59982f1a 100644 --- a/internal/api/ri_exchange_refusals_test.go +++ b/internal/api/ri_exchange_refusals_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" azurecompute "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" @@ -77,17 +78,20 @@ func TestExecuteExchange_NonUSDQuoteRefusedAs422(t *testing.T) { func TestMapExchangeErrors_Refusals(t *testing.T) { notUSD := fmt.Errorf("aborting exchange at accept time: %w", fmt.Errorf("%w: quote is in %q", exchange.ErrQuoteNotUSD, "EUR")) scope := fmt.Errorf("azure: CalculateExchange: sources[0]: %w", azurecompute.ErrUnsupportedAppliedScope) + currency := fmt.Errorf("azure: CalculateExchange: calculateExchange bills in EUR but the expected currency is USD; refusing to exchange: %w", common.ErrCurrencyNotAccepted) mappers := map[string]func(string, error) error{ "aws": mapAWSExchangeError, "azure": mapAzureExchangeError, } for name, mapErr := range mappers { - for _, refusal := range []error{notUSD, scope} { + for _, refusal := range []error{notUSD, scope, currency} { t.Run(name+"/"+refusal.Error(), func(t *testing.T) { ce, ok := IsClientError(mapErr("op failed", refusal)) require.True(t, ok) assert.Equal(t, 422, ce.code) assert.Equal(t, refusal.Error(), ce.message) + assert.Equal(t, config.RIExchangeDirectStatusFailed, directExchangeFailureStatus(ce), + "a refusal happens before the provider commit call, so the audit row is failed, not unknown") }) } t.Run(name+"/other", func(t *testing.T) { From 0a0fe9830ba2c18c9043287ca8666e56d356898c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 05:06:36 +0100 Subject: [PATCH 04/27] feat(scheduler): count recommendations skipped in a mixed Azure sweep A sweep that loses subscriptions and also drops invalid recommendations on the reachable ones now records the skipped count for the user warning and the log line. Eviction stays held back for the lost subscriptions. Co-Authored-By: Claude Opus 5.5 --- internal/scheduler/partial_skip_test.go | 31 +++++++++++++++++++++++++ internal/scheduler/partial_sweep.go | 5 ++++ internal/scheduler/scheduler.go | 10 +++++--- 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/internal/scheduler/partial_skip_test.go b/internal/scheduler/partial_skip_test.go index 36abecc6f..04e70372b 100644 --- a/internal/scheduler/partial_skip_test.go +++ b/internal/scheduler/partial_skip_test.go @@ -6,6 +6,7 @@ import ( "fmt" "testing" + azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -94,3 +95,33 @@ func TestGCPPartialResult_CountComesFromTypedMethod(t *testing.T) { require.NotNil(t, gap) assert.Equal(t, 5, gap.skippedRecommendations) } + +// A mixed Azure sweep (lost subscriptions plus recommendations dropped on the +// reachable ones) reports the skipped count for the user warning, but the lost +// subscriptions still hold stale-row eviction back. +func TestAzureMixedSweep_RecordsSkipsAndHoldsEviction(t *testing.T) { + failure := azureprovider.NewPartialSubscriptionFailure(2, + []azureprovider.SubscriptionFailure{{SubscriptionID: "sub-b", Err: errors.New("throttled")}}, + []string{"sub-a: non-USD", "sub-a: bad term"}) + + gap, err := tolerateIncompleteSweep("azure", failure) + + require.NoError(t, err) + require.NotNil(t, gap) + assert.Equal(t, 2, gap.skippedRecommendations) + assert.Equal(t, 2, gap.toProviderSweep("azure").SkippedRecommendations) + assert.True(t, gap.holdsEviction(), "lost subscriptions still hold stale-row eviction") + assert.Equal(t, 1, gap.forAccount().incompleteAccounts) +} + +func TestAzureFailureWithoutSkips_RecordsNoSkippedCount(t *testing.T) { + failure := azureprovider.NewPartialSubscriptionFailure(2, + []azureprovider.SubscriptionFailure{{SubscriptionID: "sub-b", Err: errors.New("throttled")}}, nil) + + gap, err := tolerateIncompleteSweep("azure", failure) + + require.NoError(t, err) + require.NotNil(t, gap) + assert.Zero(t, gap.skippedRecommendations) + assert.True(t, gap.holdsEviction()) +} diff --git a/internal/scheduler/partial_sweep.go b/internal/scheduler/partial_sweep.go index fc0679206..866dbdcdd 100644 --- a/internal/scheduler/partial_sweep.go +++ b/internal/scheduler/partial_sweep.go @@ -94,6 +94,11 @@ func gapFromPartialSubscriptions(p *azureprovider.PartialSubscriptionFailureErro failedScopes: max(p.Attempted-p.Succeeded, len(p.Failed)), reasons: map[string]int{}, } + // A mixed sweep also lost invalid recommendations; the count is for the + // user warning only and does not affect eviction (skippedOnly stays false). + if sk := p.Skips(); sk != nil { + g.skippedRecommendations = sk.SkippedCount() + } for _, f := range p.Failed { g.reasons[classifySweepCause(f.Err)]++ } diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index de8718730..d63fe8242 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1114,11 +1114,15 @@ func tolerateIncompleteSweep(providerName string, err error) (*sweepGap, error) if partial == nil { return tolerateSkippedRecommendations(providerName, err) } + skipped := 0 + if sk := partial.Skips(); sk != nil { + skipped = sk.SkippedCount() + } logging.Warnf( - "%s recommendations incomplete: %d of %d subscriptions succeeded; not queried: %s (keeping the %d that did, "+ - "and holding back stale-row eviction so the un-queried subscriptions' previous rows survive)", + "%s recommendations incomplete: %d of %d subscriptions succeeded; not queried: %s; %d recommendation(s) skipped as invalid "+ + "(keeping the %d that did, and holding back stale-row eviction so the un-queried subscriptions' previous rows survive)", providerName, partial.Succeeded, partial.Attempted, - strings.Join(partial.FailedSubscriptionIDs(), ", "), partial.Succeeded) + strings.Join(partial.FailedSubscriptionIDs(), ", "), skipped, partial.Succeeded) return gapFromPartialSubscriptions(partial), nil } From 937d919d212dd4040fb8ee0e6d5c40c3a69c640a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 05:07:25 +0100 Subject: [PATCH 05/27] test(api): pin the spend cap for priced Azure Advisor rows A priced USD Azure reservation has the cap applied to its total commitment, and an unpriced Savings Plan row with an empty currency is still refused. Co-Authored-By: Claude Opus 5.5 --- internal/api/purchase_currency_test.go | 33 ++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/internal/api/purchase_currency_test.go b/internal/api/purchase_currency_test.go index a0e6c161c..170cb7765 100644 --- a/internal/api/purchase_currency_test.go +++ b/internal/api/purchase_currency_test.go @@ -151,3 +151,36 @@ func TestEnforcePurchaseConstraints_NonUSDBackendErrorPropagates(t *testing.T) { assert.False(t, isClient) mockAuth.AssertNumberOfCalls(t, "HasPermissionForConstraintsAPI", 1) } + +// A priced Azure Advisor reservation (go#663) carries USD, so the cap applies +// to its total commitment (upfront plus monthly over the term), while an +// unpriced Savings Plan row with no currency is still refused under a cap. +func TestEnforcePurchaseConstraints_AzureAdvisorPricedUSDCapAppliesUnpricedSPRefused(t *testing.T) { + ctx := context.Background() + session := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd"} + monthly := 50.0 + priced := config.RecommendationRecord{ + ID: "adv-vm", Provider: "azure", Service: "compute", Region: "westeurope", ResourceType: "Standard_D2s_v3", + Count: 1, Term: 1, Payment: "monthly", UpfrontCost: 100, MonthlyCost: &monthly, CommitmentCurrency: "USD", + } + require.InDelta(t, 700, priced.TotalCommitment(), 0.001) + unpriced := config.RecommendationRecord{ + ID: "adv-sp", Provider: "azure", Service: "savingsplans", Region: "westeurope", + Count: 1, Term: 1, Payment: "monthly", UpfrontCost: 900, + } + + require.NoError(t, handlerWithCap(auth.ActionExecute, 1000).enforcePurchaseConstraints(ctx, session, auth.ActionExecute, + []config.RecommendationRecord{priced})) + + err := handlerWithCap(auth.ActionExecute, 600).enforcePurchaseConstraints(ctx, session, auth.ActionExecute, + []config.RecommendationRecord{priced}) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "exceeds the constraints", "the cap is compared against the 700 USD total, not upfront alone") + assert.NotContains(t, ce.Error(), "USD spend cap") + + requireCurrencyRefusal(t, handlerWithCap(auth.ActionExecute, 1000).enforcePurchaseConstraints(ctx, session, auth.ActionExecute, + []config.RecommendationRecord{unpriced}), "unknown") +} From 66aec97facef4d7c30125b39198a9caeeb5a9bd4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 05:08:48 +0100 Subject: [PATCH 06/27] fix(scheduler): hold stale-row eviction when Advisor pricing was unavailable A skip that means the Retail Prices catalog could not be reached (go#663) is not an invalid row, so a cycle whose Advisor rows were all skipped for it must not evict the previous cycle's rows. Other skips keep evicting. Co-Authored-By: Claude Opus 5.5 --- ...advisor_pricing_outage_integration_test.go | 103 ++++++++++++++++++ internal/scheduler/partial_skip_test.go | 28 +++++ internal/scheduler/partial_sweep.go | 33 +++++- 3 files changed, 163 insertions(+), 1 deletion(-) create mode 100644 internal/scheduler/advisor_pricing_outage_integration_test.go diff --git a/internal/scheduler/advisor_pricing_outage_integration_test.go b/internal/scheduler/advisor_pricing_outage_integration_test.go new file mode 100644 index 000000000..7d91b81de --- /dev/null +++ b/internal/scheduler/advisor_pricing_outage_integration_test.go @@ -0,0 +1,103 @@ +//go:build integration + +package scheduler + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Skip reasons as the go#663 Advisor pricing records them (see pricingOutageReasons). +var advisorOutageSkips = []string{ + `advisor: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, + `advisor: price lookup unavailable: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, +} + +// collectAzureAmbientCycle runs one ambient Azure collection against store +// and persists it the way a scheduled cycle does. +func collectAzureAmbientCycle(t *testing.T, store config.StoreInterface, recs []common.Recommendation, sweepErr error) []config.RecommendationRecord { + t.Helper() + t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-a") + ctx := context.Background() + + recClient := new(MockRecommendationsClient) + recClient.On("GetAllRecommendations", mock.Anything).Return(recs, sweepErr).Once() + prov := new(MockProvider) + prov.On("GetRecommendationsClient", mock.Anything).Return(recClient, nil).Once() + factory := new(MockProviderFactory) + factory.On("CreateAndValidateProvider", mock.Anything, "azure", mock.Anything).Return(prov, nil).Once() + s := &Scheduler{config: store, providerFactory: factory} + + got, ids, _, err := s.collectAzureRecommendations(ctx, &config.GlobalConfig{}) + require.NoError(t, err) + s.persistCollection(ctx, got, expandSuccessfulCollects("azure", ids), nil, nil) + return got +} + +func storedAzureIDs(t *testing.T, store config.StoreInterface) []string { + t.Helper() + rows, err := store.ListStoredRecommendations(context.Background(), config.RecommendationFilter{}) + require.NoError(t, err) + ids := []string{} + for _, row := range rows { + ids = append(ids, row.ID) + } + return ids +} + +func seedAdvisorRow(t *testing.T, store config.StoreInterface) { + t.Helper() + require.NoError(t, store.UpsertRecommendations(context.Background(), time.Now().Add(-time.Hour), []config.RecommendationRecord{ + {ID: "prev-advisor-row", Provider: "azure", Service: "compute", Region: "westeurope", ResourceType: "Standard_D2s_v3", + Savings: 40, Count: 1, Term: 12, Payment: "upfront"}, + }, nil)) +} + +// An Advisor pricing outage skips every row; the previous cycle's rows must +// survive it, a later good cycle replaces them, and a genuine empty cycle +// still evicts. +func TestAzureAdvisorPricingOutage_HoldsEviction(t *testing.T) { + ctx := context.Background() + newStore := func(t *testing.T) config.StoreInterface { + container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(container.DB) + seedAdvisorRow(t, store) + return store + } + + t.Run("outage keeps prior rows", func(t *testing.T) { + store := newStore(t) + collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips}) + require.Contains(t, storedAzureIDs(t, store), "prev-advisor-row") + }) + + t.Run("later good cycle replaces them", func(t *testing.T) { + store := newStore(t) + collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips}) + got := collectAzureAmbientCycle(t, store, []common.Recommendation{partialSweepRec()}, nil) + require.NotEmpty(t, got) + ids := storedAzureIDs(t, store) + require.NotContains(t, ids, "prev-advisor-row") + require.Len(t, ids, len(got)) + }) + + t.Run("genuine empty cycle still evicts", func(t *testing.T) { + store := newStore(t) + collectAzureAmbientCycle(t, store, nil, nil) + require.NotContains(t, storedAzureIDs(t, store), "prev-advisor-row") + }) + + t.Run("unrelated skip reasons still evict", func(t *testing.T) { + store := newStore(t) + collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: []string{"advisor: row is in EUR, not USD"}}) + require.NotContains(t, storedAzureIDs(t, store), "prev-advisor-row") + }) +} diff --git a/internal/scheduler/partial_skip_test.go b/internal/scheduler/partial_skip_test.go index 04e70372b..5f3cfcf65 100644 --- a/internal/scheduler/partial_skip_test.go +++ b/internal/scheduler/partial_skip_test.go @@ -125,3 +125,31 @@ func TestAzureFailureWithoutSkips_RecordsNoSkippedCount(t *testing.T) { assert.Zero(t, gap.skippedRecommendations) assert.True(t, gap.holdsEviction()) } + +func TestHasPricingOutageSkip(t *testing.T) { + for name, tc := range map[string]struct { + reasons []string + want bool + }{ + "first failed row": {[]string{"advisor: azure advisor: advisor compute reservation price: get: retail prices fetch failed"}, true}, + "later rows": {[]string{"advisor: non-USD", "advisor: price lookup unavailable: x"}, true}, + "invalid recommendation": {[]string{"advisor: row is in EUR, not USD", "computeengine us-east1: non-USD"}, false}, + "mid-string mention": {[]string{"computeengine: price lookup unavailable: x"}, false}, + "none": {nil, false}, + } { + t.Run(name, func(t *testing.T) { assert.Equal(t, tc.want, hasPricingOutageSkip(tc.reasons)) }) + } +} + +// A pricing outage holds eviction; ordinary invalid-row skips do not. +func TestTolerateSkipped_PricingOutageHoldsEviction(t *testing.T) { + gap, err := tolerateIncompleteSweep("azure", &skipErr{partial: true, count: 1, + reasons: []string{"advisor: price lookup unavailable: x"}}) + require.NoError(t, err) + assert.True(t, gap.holdsEviction()) + assert.Equal(t, 1, gap.skippedRecommendations) + + gap, err = tolerateIncompleteSweep("azure", &skipErr{partial: true, count: 1, reasons: []string{"advisor: non-USD"}}) + require.NoError(t, err) + assert.False(t, gap.holdsEviction()) +} diff --git a/internal/scheduler/partial_sweep.go b/internal/scheduler/partial_sweep.go index 866dbdcdd..7b8d1a3fe 100644 --- a/internal/scheduler/partial_sweep.go +++ b/internal/scheduler/partial_sweep.go @@ -68,7 +68,38 @@ func tolerateSkippedRecommendations(providerName string, err error) (*sweepGap, return nil, err } logging.Warnf("%s recommendations partial, keeping the rest: %v", providerName, err) - return &sweepGap{skippedRecommendations: p.SkippedCount(), skippedOnly: true, reasons: map[string]int{}}, nil + return &sweepGap{ + skippedRecommendations: p.SkippedCount(), + // A skip that means "the price could not be fetched" says nothing about + // the row being invalid, so its previous-cycle row must survive. + skippedOnly: !hasPricingOutageSkip(p.SkipReasons()), + reasons: map[string]int{}, + }, nil +} + +// Skip-reason prefixes cloud-commitments-go records when Azure Advisor rows +// cannot be priced because the Retail Prices catalog is unreachable +// (providers/azure/recommendations_advisor_pricing.go, priceAdvisorReservations, +// go#663). The library exports no constant for them: RecordSkip stores +// ": " strings. The first failed row is recorded with the +// collection-failure wrapper "azure advisor: "; every later row of the call +// gets "price lookup unavailable: ". The wrapper also marks a truncated +// Advisor walk, whose missing rows must survive too. +var pricingOutageSkipPrefixes = []string{ + "advisor: price lookup unavailable: ", + "advisor: azure advisor: ", +} + +// hasPricingOutageSkip reports whether any skip reason is a pricing outage. +func hasPricingOutageSkip(reasons []string) bool { + for _, reason := range reasons { + for _, prefix := range pricingOutageSkipPrefixes { + if strings.HasPrefix(reason, prefix) { + return true + } + } + } + return false } // gapFromAWSIncomplete summarizes an AWS IncompleteRecommendationsError. From 76b53cd4187c6835ffd2db2e8c561ec3c0c88c99 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 05:19:53 +0100 Subject: [PATCH 07/27] test(scheduler): preallocate the stored-id slice in the outage test Satisfies the prealloc linter, which only sees this file under the integration build tag. Co-Authored-By: Claude Opus 5.5 --- internal/scheduler/advisor_pricing_outage_integration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/scheduler/advisor_pricing_outage_integration_test.go b/internal/scheduler/advisor_pricing_outage_integration_test.go index 7d91b81de..466cf071a 100644 --- a/internal/scheduler/advisor_pricing_outage_integration_test.go +++ b/internal/scheduler/advisor_pricing_outage_integration_test.go @@ -46,7 +46,7 @@ func storedAzureIDs(t *testing.T, store config.StoreInterface) []string { t.Helper() rows, err := store.ListStoredRecommendations(context.Background(), config.RecommendationFilter{}) require.NoError(t, err) - ids := []string{} + ids := make([]string, 0, len(rows)) for _, row := range rows { ids = append(ids, row.ID) } From 5e3a11d7082904957fe77bf0b87705875887e5e6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 05:43:58 +0100 Subject: [PATCH 08/27] fix(scheduler): keep evicting after a truncated Advisor walk The "azure advisor: " skip prefix also marks a walk cut off at the page cap, which is not a pricing outage. Holding eviction for it kept that account's stale rows forever. Exclude the truncation reason and pin a mid-string prefix case. Co-Authored-By: Claude Opus 5.5 --- internal/scheduler/partial_skip_test.go | 13 ++++++++----- internal/scheduler/partial_sweep.go | 14 ++++++++++++-- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/internal/scheduler/partial_skip_test.go b/internal/scheduler/partial_skip_test.go index 5f3cfcf65..7bb73fd04 100644 --- a/internal/scheduler/partial_skip_test.go +++ b/internal/scheduler/partial_skip_test.go @@ -131,11 +131,14 @@ func TestHasPricingOutageSkip(t *testing.T) { reasons []string want bool }{ - "first failed row": {[]string{"advisor: azure advisor: advisor compute reservation price: get: retail prices fetch failed"}, true}, - "later rows": {[]string{"advisor: non-USD", "advisor: price lookup unavailable: x"}, true}, - "invalid recommendation": {[]string{"advisor: row is in EUR, not USD", "computeengine us-east1: non-USD"}, false}, - "mid-string mention": {[]string{"computeengine: price lookup unavailable: x"}, false}, - "none": {nil, false}, + "first failed row": {[]string{"advisor: azure advisor: advisor compute reservation price: get: retail prices fetch failed"}, true}, + "later rows": {[]string{"advisor: non-USD", "advisor: price lookup unavailable: x"}, true}, + "invalid recommendation": {[]string{"advisor: row is in EUR, not USD", "computeengine us-east1: non-USD"}, false}, + "mid-string mention": {[]string{"computeengine: price lookup unavailable: x"}, false}, + "truncated walk is not an outage": {[]string{"advisor: azure advisor: recommendation list is incomplete"}, false}, + "truncated walk plus outage": {[]string{"advisor: azure advisor: recommendation list is incomplete", "advisor: azure advisor: get: retail prices fetch failed"}, true}, + "prefix mid-string": {[]string{"sub-1: advisor: price lookup unavailable: x"}, false}, + "none": {nil, false}, } { t.Run(name, func(t *testing.T) { assert.Equal(t, tc.want, hasPricingOutageSkip(tc.reasons)) }) } diff --git a/internal/scheduler/partial_sweep.go b/internal/scheduler/partial_sweep.go index 7b8d1a3fe..173f409d3 100644 --- a/internal/scheduler/partial_sweep.go +++ b/internal/scheduler/partial_sweep.go @@ -83,16 +83,26 @@ func tolerateSkippedRecommendations(providerName string, err error) (*sweepGap, // go#663). The library exports no constant for them: RecordSkip stores // ": " strings. The first failed row is recorded with the // collection-failure wrapper "azure advisor: "; every later row of the call -// gets "price lookup unavailable: ". The wrapper also marks a truncated -// Advisor walk, whose missing rows must survive too. +// gets "price lookup unavailable: ". var pricingOutageSkipPrefixes = []string{ "advisor: price lookup unavailable: ", "advisor: azure advisor: ", } +// advisorTruncatedSkipPrefix starts the skip recorded for an Advisor walk that +// hit the page cap or a page error (collectAdvisorOrSkip, +// providers/azure/recommendations.go:471 at 61111428). It shares the +// "advisor: azure advisor: " prefix but is not a pricing outage: a walk over +// the page cap is always truncated, so holding eviction would keep that +// account's stale rows forever. +const advisorTruncatedSkipPrefix = "advisor: azure advisor: recommendation list is incomplete" + // hasPricingOutageSkip reports whether any skip reason is a pricing outage. func hasPricingOutageSkip(reasons []string) bool { for _, reason := range reasons { + if strings.HasPrefix(reason, advisorTruncatedSkipPrefix) { + continue + } for _, prefix := range pricingOutageSkipPrefixes { if strings.HasPrefix(reason, prefix) { return true From b35bac3e4d0612d3328a0430e79748ed998f35ea Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 05:44:08 +0100 Subject: [PATCH 09/27] fix(scheduler): drop "as invalid" from the mixed-sweep skip log Skipped recommendations now include pricing-outage skips, so the log text no longer claims they were invalid. Co-Authored-By: Claude Opus 5.5 --- internal/scheduler/scheduler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index d63fe8242..8462ec877 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1119,7 +1119,7 @@ func tolerateIncompleteSweep(providerName string, err error) (*sweepGap, error) skipped = sk.SkippedCount() } logging.Warnf( - "%s recommendations incomplete: %d of %d subscriptions succeeded; not queried: %s; %d recommendation(s) skipped as invalid "+ + "%s recommendations incomplete: %d of %d subscriptions succeeded; not queried: %s; %d recommendation(s) skipped "+ "(keeping the %d that did, and holding back stale-row eviction so the un-queried subscriptions' previous rows survive)", providerName, partial.Succeeded, partial.Attempted, strings.Join(partial.FailedSubscriptionIDs(), ", "), skipped, partial.Succeeded) From 41988bf8361ec383a3854889a92e0548971059cc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 05:49:41 +0100 Subject: [PATCH 10/27] feat(scheduler): record accounts held through an Advisor pricing outage An account whose previous-cycle Advisor rows are kept because the price catalog was unreachable now counts as incomplete with the closed reason pricing_unavailable, and its platform account ID is listed in the persisted partial-sweep state, the freshness API, the OpenAPI schema and the frontend type. The purchase path reads it in the next commit. Co-Authored-By: Claude Opus 5.5 --- frontend/src/api/recommendations.ts | 2 + internal/api/openapi.yaml | 12 ++++- internal/config/types.go | 9 ++++ ...advisor_pricing_outage_integration_test.go | 32 +++++++++++- internal/scheduler/partial_skip_test.go | 24 +++++++++ internal/scheduler/partial_sweep.go | 49 ++++++++++++++++--- internal/scheduler/scheduler.go | 8 +-- 7 files changed, 121 insertions(+), 15 deletions(-) diff --git a/frontend/src/api/recommendations.ts b/frontend/src/api/recommendations.ts index 810c48056..6b8f6ac26 100644 --- a/frontend/src/api/recommendations.ts +++ b/frontend/src/api/recommendations.ts @@ -85,6 +85,8 @@ export interface PartialProviderSweep { /** Recommendations the provider dropped as invalid (GCP: non-USD, malformed, unknown term). */ skipped_recommendations?: number; reasons: Record; + /** Platform account IDs whose rows are held through a pricing outage (reason `pricing_unavailable`). */ + held_account_ids?: string[]; } export interface RecommendationsFreshness { diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index ef4e29b36..fd99a0507 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -7026,12 +7026,22 @@ components: description: > Failure count per reason category. Keys are drawn from a closed set: permission_denied, throttled, timeout, canceled, not_found, - provider_unavailable, other. + provider_unavailable, pricing_unavailable, other. additionalProperties: type: integer example: permission_denied: 2 throttled: 1 + held_account_ids: + type: array + description: > + Platform cloud-account IDs (not cloud account numbers) whose + previous-cycle Azure Advisor rows are held because the price + catalog was unreachable (reason pricing_unavailable). Their + reservations cannot be purchased until a collection re-prices them. + items: + type: string + format: uuid # -- Plans -------------------------------------------------------------- PlansResponse: diff --git a/internal/config/types.go b/internal/config/types.go index 44a8cb09a..e2577c84b 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -949,6 +949,11 @@ const ( PartialReasonNotFound = "not_found" PartialReasonProviderUnavailable = "provider_unavailable" PartialReasonOther = "other" + // PartialReasonPricingUnavailable marks accounts whose previous-cycle + // Advisor rows were kept because the Retail Prices catalog could not be + // reached. Those rows may carry an outdated price and must not be + // purchased until a good collection re-prices them. + PartialReasonPricingUnavailable = "pricing_unavailable" ) // PartialProviderSweep summarizes one provider's incomplete sweeps in a @@ -973,6 +978,10 @@ type PartialProviderSweep struct { SkippedRecommendations int `json:"skipped_recommendations"` // Reasons counts the failure causes by PartialReason* category. Reasons map[string]int `json:"reasons"` + // HeldAccountIDs lists the platform cloud-account UUIDs (not cloud + // account numbers) whose previous-cycle rows are held through a pricing + // outage; the purchase path refuses their recommendations. + HeldAccountIDs []string `json:"held_account_ids,omitempty"` } // SuccessfulCollect identifies a (provider, account) pair whose collection diff --git a/internal/scheduler/advisor_pricing_outage_integration_test.go b/internal/scheduler/advisor_pricing_outage_integration_test.go index 466cf071a..d0e925a36 100644 --- a/internal/scheduler/advisor_pricing_outage_integration_test.go +++ b/internal/scheduler/advisor_pricing_outage_integration_test.go @@ -11,6 +11,7 @@ import ( azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/google/uuid" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -36,9 +37,9 @@ func collectAzureAmbientCycle(t *testing.T, store config.StoreInterface, recs [] factory.On("CreateAndValidateProvider", mock.Anything, "azure", mock.Anything).Return(prov, nil).Once() s := &Scheduler{config: store, providerFactory: factory} - got, ids, _, err := s.collectAzureRecommendations(ctx, &config.GlobalConfig{}) + got, ids, gap, err := s.collectAzureRecommendations(ctx, &config.GlobalConfig{}) require.NoError(t, err) - s.persistCollection(ctx, got, expandSuccessfulCollects("azure", ids), nil, nil) + s.persistCollection(ctx, got, expandSuccessfulCollects("azure", ids), nil, appendPartialProvider(nil, "azure", gap)) return got } @@ -101,3 +102,30 @@ func TestAzureAdvisorPricingOutage_HoldsEviction(t *testing.T) { require.NotContains(t, storedAzureIDs(t, store), "prev-advisor-row") }) } + +// The freshness payload names the account whose Advisor rows are held through +// a pricing outage, with the closed pricing_unavailable reason, and a good +// cycle clears it. The purchase path reads this to refuse the held rows. +func TestAzureAdvisorPricingOutage_FreshnessListsHeldAccount(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(container.DB) + acct := config.CloudAccount{ID: uuid.NewString(), Name: "host", Provider: "azure", ExternalID: "sub-a", AzureSubscriptionID: "sub-a", Enabled: false} + require.NoError(t, store.CreateCloudAccount(ctx, &acct)) + + collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips}) + fresh, err := store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.NotNil(t, fresh.LastCollectionPartial) + require.Len(t, fresh.LastCollectionPartial.Providers, 1) + sweep := fresh.LastCollectionPartial.Providers[0] + require.Equal(t, "azure", sweep.Provider) + require.Equal(t, []string{acct.ID}, sweep.HeldAccountIDs) + require.Equal(t, 1, sweep.IncompleteAccounts) + require.Positive(t, sweep.Reasons[config.PartialReasonPricingUnavailable]) + + collectAzureAmbientCycle(t, store, []common.Recommendation{partialSweepRec()}, nil) + fresh, err = store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.Nil(t, fresh.LastCollectionPartial, "a good cycle clears the hold") +} diff --git a/internal/scheduler/partial_skip_test.go b/internal/scheduler/partial_skip_test.go index 7bb73fd04..e56da1b4d 100644 --- a/internal/scheduler/partial_skip_test.go +++ b/internal/scheduler/partial_skip_test.go @@ -156,3 +156,27 @@ func TestTolerateSkipped_PricingOutageHoldsEviction(t *testing.T) { require.NoError(t, err) assert.False(t, gap.holdsEviction()) } + +// The held account's platform ID travels with the gap through the fan-out; +// accounts whose skips are ordinary invalid rows are not held. +func TestPricingOutageGap_FanOutRecordsHeldAccounts(t *testing.T) { + accounts := []config.CloudAccount{{ID: "held", Name: "held"}, {ID: "plain", Name: "plain"}} + fn := func(_ context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { + reason := "advisor: non-USD" + if acct.ID == "held" { + reason = "advisor: price lookup unavailable: x" + } + gap, err := tolerateIncompleteSweep("azure", &skipErr{partial: true, count: 1, reasons: []string{reason}}) + return []config.RecommendationRecord{{ID: acct.ID}}, gap, err + } + + _, outcome := fanOutPerAccount(context.Background(), "Azure", accounts, fn) + + require.NotNil(t, outcome.Gap) + sweep := outcome.Gap.toProviderSweep("azure") + assert.Equal(t, []string{"held"}, sweep.HeldAccountIDs) + assert.Equal(t, 1, sweep.IncompleteAccounts) + assert.Equal(t, 1, sweep.Reasons[config.PartialReasonPricingUnavailable]) + assert.Equal(t, []string{"held"}, outcome.IncompleteAccountIDs) + assert.Equal(t, []string{"plain"}, outcome.SucceededAccountIDs) +} diff --git a/internal/scheduler/partial_sweep.go b/internal/scheduler/partial_sweep.go index 173f409d3..f50d86f71 100644 --- a/internal/scheduler/partial_sweep.go +++ b/internal/scheduler/partial_sweep.go @@ -4,6 +4,7 @@ import ( "context" "errors" "net/http" + "slices" "strings" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -39,6 +40,12 @@ type sweepGap struct { // but invalid recommendations, so the account's sweep itself was complete. skippedOnly bool reasons map[string]int + // pricingUnavailable is set when a skip meant the price could not be + // fetched, so the previous cycle's rows are held (see hasPricingOutageSkip). + pricingUnavailable bool + // heldAccountIDs are the platform account UUIDs whose rows are held for + // that reason; the collector that knows the account fills them in. + heldAccountIDs []string } // holdsEviction reports whether the sweep must stay out of the succeeded- @@ -68,13 +75,30 @@ func tolerateSkippedRecommendations(providerName string, err error) (*sweepGap, return nil, err } logging.Warnf("%s recommendations partial, keeping the rest: %v", providerName, err) - return &sweepGap{ + // A skip that means "the price could not be fetched" says nothing about + // the row being invalid, so its previous-cycle row must survive. + outage := countPricingOutageSkips(p.SkipReasons()) + g := &sweepGap{ skippedRecommendations: p.SkippedCount(), - // A skip that means "the price could not be fetched" says nothing about - // the row being invalid, so its previous-cycle row must survive. - skippedOnly: !hasPricingOutageSkip(p.SkipReasons()), - reasons: map[string]int{}, - }, nil + skippedOnly: outage == 0, + pricingUnavailable: outage > 0, + reasons: map[string]int{}, + } + if outage > 0 { + g.reasons[config.PartialReasonPricingUnavailable] = outage + } + return g, nil +} + +// withHeld records accountID as held when the gap is a pricing outage. An +// empty accountID (an unresolved ambient identity) cannot be named. +func (g *sweepGap) withHeld(accountID string) *sweepGap { + if g == nil || !g.pricingUnavailable || accountID == "" { + return g + } + out := mergeSweepGaps(nil, g) + out.heldAccountIDs = append(out.heldAccountIDs, accountID) + return out } // Skip-reason prefixes cloud-commitments-go records when Azure Advisor rows @@ -99,17 +123,23 @@ const advisorTruncatedSkipPrefix = "advisor: azure advisor: recommendation list // hasPricingOutageSkip reports whether any skip reason is a pricing outage. func hasPricingOutageSkip(reasons []string) bool { + return countPricingOutageSkips(reasons) > 0 +} + +func countPricingOutageSkips(reasons []string) int { + n := 0 for _, reason := range reasons { if strings.HasPrefix(reason, advisorTruncatedSkipPrefix) { continue } for _, prefix := range pricingOutageSkipPrefixes { if strings.HasPrefix(reason, prefix) { - return true + n++ + break } } } - return false + return n } // gapFromAWSIncomplete summarizes an AWS IncompleteRecommendationsError. @@ -172,6 +202,8 @@ func mergeSweepGaps(a, b *sweepGap) *sweepGap { continue } out.skippedOnly = out.skippedOnly && g.skippedOnly + out.pricingUnavailable = out.pricingUnavailable || g.pricingUnavailable + out.heldAccountIDs = append(out.heldAccountIDs, g.heldAccountIDs...) out.skippedRecommendations += g.skippedRecommendations out.incompleteAccounts += g.incompleteAccounts out.attemptedScopes += g.attemptedScopes @@ -198,6 +230,7 @@ func (g *sweepGap) toProviderSweep(providerName string) config.PartialProviderSw FailedDetails: g.failedDetails, SkippedRecommendations: g.skippedRecommendations, Reasons: reasons, + HeldAccountIDs: slices.Sorted(slices.Values(g.heldAccountIDs)), } } diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 8462ec877..8d4f48a8c 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -567,7 +567,7 @@ func (s *Scheduler) collectAWSRecommendations(ctx context.Context, globalCfg *co // in those cases. if acctID := s.resolveAmbientHostAccountID(ctx); acctID != "" { recs = s.tagAccount(recs, acctID) - return recs, ambientEvictionRoster(!gap.holdsEviction(), acctID), gap.forAccount(), nil + return recs, ambientEvictionRoster(!gap.holdsEviction(), acctID), gap.forAccount().withHeld(acctID), nil } return recs, ambientEvictionRoster(!gap.holdsEviction(), ""), gap.forAccount(), nil } @@ -678,7 +678,7 @@ func fanOutPerAccount( } else { outcome.SucceededAccountIDs = append(outcome.SucceededAccountIDs, acct.ID) } - outcome.Gap = mergeSweepGaps(outcome.Gap, gap.forAccount()) + outcome.Gap = mergeSweepGaps(outcome.Gap, gap.forAccount().withHeld(acct.ID)) mu.Unlock() return nil }) @@ -918,7 +918,7 @@ func (s *Scheduler) collectAzureRecommendations(ctx context.Context, _ *config.G } if acctID := s.resolveAmbientAccountID(ctx, "azure", subscriptionID); acctID != "" { recs = s.tagAccount(recs, acctID) - return recs, ambientEvictionRoster(!gap.holdsEviction(), acctID), gap.forAccount(), nil + return recs, ambientEvictionRoster(!gap.holdsEviction(), acctID), gap.forAccount().withHeld(acctID), nil } return recs, ambientEvictionRoster(!gap.holdsEviction(), ""), gap.forAccount(), nil } @@ -993,7 +993,7 @@ func (s *Scheduler) collectGCPRecommendations(ctx context.Context, _ *config.Glo } if acctID := s.resolveAmbientAccountID(ctx, "gcp", projectID); acctID != "" { recs = s.tagAccount(recs, acctID) - return recs, ambientEvictionRoster(!gap.holdsEviction(), acctID), gap.forAccount(), nil + return recs, ambientEvictionRoster(!gap.holdsEviction(), acctID), gap.forAccount().withHeld(acctID), nil } return recs, ambientEvictionRoster(!gap.holdsEviction(), ""), gap.forAccount(), nil } From 68f28725333574bfbe80bd7fe9421b55c99d9a0b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 05:51:41 +0100 Subject: [PATCH 11/27] fix(purchase): refuse Azure recs whose prices are held through a pricing outage Held Advisor rows are the previous cycle's, so they match the approved execution while their price may be days old. Read the latest collection state and refuse Azure recs of a held account with ErrStaleAzurePricing until a good collection re-prices them. Fail closed when the state cannot be read. Co-Authored-By: Claude Opus 5.5 --- internal/purchase/azure_pricing.go | 33 ++++++++++ internal/purchase/azure_pricing_hold_test.go | 66 ++++++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 internal/purchase/azure_pricing_hold_test.go diff --git a/internal/purchase/azure_pricing.go b/internal/purchase/azure_pricing.go index b328ff35c..cef18a4d0 100644 --- a/internal/purchase/azure_pricing.go +++ b/internal/purchase/azure_pricing.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "math" + "slices" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" ) @@ -48,9 +49,41 @@ func (m *Manager) staleAzurePricingRefusal(ctx context.Context, exec *config.Pur return fmt.Errorf("%w: Azure recommendation %q no longer has the approved price", ErrStaleAzurePricing, rec.ID) } } + return m.heldAzurePricingRefusal(ctx, azure) +} + +// heldAzurePricingRefusal refuses Azure recs whose account's stored rows are +// held through a Retail Prices outage. Those rows are the previous cycle's, so +// they match the approved execution while their price may be days old. +// A rec without an account (an unresolved ambient identity) cannot be named in +// the hold, so any current Azure hold refuses it. Fails closed when the +// collection state cannot be read. +func (m *Manager) heldAzurePricingRefusal(ctx context.Context, azure []config.RecommendationRecord) error { + fresh, err := m.config.GetRecommendationsFreshness(ctx) + if err != nil || fresh == nil { + return fmt.Errorf("%w: cannot read the latest collection state to confirm Azure prices are current: %v", ErrStaleAzurePricing, err) + } + if fresh.LastCollectionPartial == nil { + return nil + } + for _, sweep := range fresh.LastCollectionPartial.Providers { + if sweep.Provider != "azure" || sweep.Reasons[config.PartialReasonPricingUnavailable] == 0 { + continue + } + for i := range azure { + if azureAccountHeld(azure[i].CloudAccountID, sweep.HeldAccountIDs) { + return fmt.Errorf("%w: Azure prices for recommendation %q could not be refreshed (price catalog unavailable); "+ + "refresh recommendations after prices recover and submit a new purchase", ErrStaleAzurePricing, azure[i].ID) + } + } + } return nil } +func azureAccountHeld(accountID *string, held []string) bool { + return accountID == nil || *accountID == "" || slices.Contains(held, *accountID) +} + func azurePricingMatches(rec, stored config.RecommendationRecord) bool { if rec.Count <= 0 || stored.Count <= 0 { return false diff --git a/internal/purchase/azure_pricing_hold_test.go b/internal/purchase/azure_pricing_hold_test.go new file mode 100644 index 000000000..963bbffaa --- /dev/null +++ b/internal/purchase/azure_pricing_hold_test.go @@ -0,0 +1,66 @@ +package purchase + +import ( + "context" + "errors" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func heldFreshness(heldIDs ...string) *config.RecommendationsFreshness { + return &config.RecommendationsFreshness{LastCollectionPartial: &config.RecommendationsCollectionPartial{ + Providers: []config.PartialProviderSweep{{ + Provider: "azure", IncompleteAccounts: len(heldIDs), HeldAccountIDs: heldIDs, + Reasons: map[string]int{config.PartialReasonPricingUnavailable: 1}, + }}, + }} +} + +// An Advisor row held through a pricing outage still matches the approved +// execution, because both carry the same old price. The collection state is +// what tells the purchase path the price may be days old. +func TestAzurePricingHold(t *testing.T) { + acct := "11111111-1111-1111-1111-111111111111" + other := "22222222-2222-2222-2222-222222222222" + cases := []struct { + name string + execAcct *string + fresh *config.RecommendationsFreshness + freshErr error + wantStale bool + }{ + {"held account refused", &acct, heldFreshness(acct), nil, true}, + {"hold cleared by a good cycle", &acct, &config.RecommendationsFreshness{}, nil, false}, + {"unheld account unaffected", &other, heldFreshness(acct), nil, false}, + {"unreadable hold state refuses", &acct, nil, errors.New("database unavailable"), true}, + {"unresolved ambient rec refused during an outage", nil, heldFreshness(), nil, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + base := config.RecommendationRecord{ID: "azure-held", Provider: "azure", Service: "compute", Region: "westeurope", + ResourceType: "Standard_D2s_v3", Count: 2, Term: 3, UpfrontCost: 200, Selected: true, CloudAccountID: tc.execAcct} + exec := armedExecution("azure", "compute", 0, "approved") + exec.Recommendations = []config.RecommendationRecord{base} + mgr, store, recorder := armedHarness(t, common.ServiceCompute) + store.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}). + Return([]config.RecommendationRecord{base}, nil).Maybe() + store.On("GetRecommendationsFreshness", mock.Anything).Return(tc.fresh, tc.freshErr).Maybe() + + err := mgr.executeAndFinalize(context.Background(), exec) + + if tc.wantStale { + require.ErrorIs(t, err, ErrStaleAzurePricing) + assert.Zero(t, recorder.count()) + assert.Equal(t, "failed", exec.Status) + return + } + require.NoError(t, err) + assert.Equal(t, 1, recorder.count()) + }) + } +} From bd62ed0ed603fca6f0614eefa1590552de5f3669 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 05:56:39 +0100 Subject: [PATCH 12/27] fix(purchase): wrap the collection-state read error Satisfies errorlint and keeps the cause matchable; a nil state now has its own message. Co-Authored-By: Claude Opus 5.5 --- internal/purchase/azure_pricing.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/purchase/azure_pricing.go b/internal/purchase/azure_pricing.go index cef18a4d0..c5b828b30 100644 --- a/internal/purchase/azure_pricing.go +++ b/internal/purchase/azure_pricing.go @@ -60,8 +60,11 @@ func (m *Manager) staleAzurePricingRefusal(ctx context.Context, exec *config.Pur // collection state cannot be read. func (m *Manager) heldAzurePricingRefusal(ctx context.Context, azure []config.RecommendationRecord) error { fresh, err := m.config.GetRecommendationsFreshness(ctx) - if err != nil || fresh == nil { - return fmt.Errorf("%w: cannot read the latest collection state to confirm Azure prices are current: %v", ErrStaleAzurePricing, err) + if err != nil { + return fmt.Errorf("%w: cannot read the latest collection state to confirm Azure prices are current: %w", ErrStaleAzurePricing, err) + } + if fresh == nil { + return fmt.Errorf("%w: the latest collection state is missing, so Azure prices cannot be confirmed current", ErrStaleAzurePricing) } if fresh.LastCollectionPartial == nil { return nil From 8cbdddd7c92a5e44aa11ac6963c48e55943850e4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 06:04:41 +0100 Subject: [PATCH 13/27] fix(scheduler): carry the Azure pricing hold across collection cycles UpsertRecommendations resets last_collection_partial every cycle, so an outright Azure failure, a run without Azure or an outage on another account dropped the hold while the held rows were still at their old price. Merge the previous hold into each cycle: drop accounts re-priced now, add new ones, keep the rest. If the previous state cannot be read, persist nothing so the marker stays. Also pins that an accountless rec is refused during any Azure hold. Co-Authored-By: Claude Opus 5.5 --- internal/purchase/azure_pricing_hold_test.go | 3 + internal/scheduler/azure_hold_carry.go | 97 ++++++++++++++++ .../azure_hold_carry_integration_test.go | 105 ++++++++++++++++++ internal/scheduler/azure_hold_carry_test.go | 72 ++++++++++++ internal/scheduler/scheduler.go | 9 ++ 5 files changed, 286 insertions(+) create mode 100644 internal/scheduler/azure_hold_carry.go create mode 100644 internal/scheduler/azure_hold_carry_integration_test.go create mode 100644 internal/scheduler/azure_hold_carry_test.go diff --git a/internal/purchase/azure_pricing_hold_test.go b/internal/purchase/azure_pricing_hold_test.go index 963bbffaa..8eac27227 100644 --- a/internal/purchase/azure_pricing_hold_test.go +++ b/internal/purchase/azure_pricing_hold_test.go @@ -39,6 +39,9 @@ func TestAzurePricingHold(t *testing.T) { {"unheld account unaffected", &other, heldFreshness(acct), nil, false}, {"unreadable hold state refuses", &acct, nil, errors.New("database unavailable"), true}, {"unresolved ambient rec refused during an outage", nil, heldFreshness(), nil, true}, + // Fail-closed by design: a rec without an account cannot be matched to the + // hold list, so any Azure hold refuses it. + {"accountless rec refused while other accounts are held", nil, heldFreshness(acct), nil, true}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/scheduler/azure_hold_carry.go b/internal/scheduler/azure_hold_carry.go new file mode 100644 index 000000000..6626579aa --- /dev/null +++ b/internal/scheduler/azure_hold_carry.go @@ -0,0 +1,97 @@ +package scheduler + +import ( + "context" + "fmt" + "slices" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// carryForwardAzureHold keeps the Azure pricing hold alive for as long as the +// held rows are. UpsertRecommendations resets last_collection_partial on every +// cycle, but an account's Advisor rows stay at their old price until a later +// cycle re-prices them, so the previous hold is merged into this cycle's state: +// accounts re-priced now (collected with no outage skips, hence in collects) +// drop out, newly held ones are added, the rest carry over. That covers +// another outage, an outright Azure failure and a run that skips Azure. +// +// A hold with no account IDs (an unresolved ambient identity) carries until an +// ambient Azure collection succeeds. An error reading the previous state is +// returned: the caller must not upsert, because that would reset the marker. +func (s *Scheduler) carryForwardAzureHold(ctx context.Context, partial *config.RecommendationsCollectionPartial, collects []config.SuccessfulCollect) (*config.RecommendationsCollectionPartial, error) { + fresh, err := s.config.GetRecommendationsFreshness(ctx) + if err != nil { + return nil, fmt.Errorf("read previous pricing hold: %w", err) + } + if fresh == nil || fresh.LastCollectionPartial == nil { + return partial, nil + } + prev := previousAzureHold(fresh.LastCollectionPartial) + if prev == nil { + return partial, nil + } + carried, unresolved := stillHeld(prev, collects) + if len(carried) == 0 && !unresolved { + return partial, nil + } + return mergeCarriedHold(partial, carried), nil +} + +// mergeCarriedHold adds the carried account IDs and the pricing_unavailable +// reason to partial's Azure entry, creating the entry when this cycle had none. +func mergeCarriedHold(partial *config.RecommendationsCollectionPartial, carried []string) *config.RecommendationsCollectionPartial { + if partial == nil { + partial = &config.RecommendationsCollectionPartial{} + } + idx := slices.IndexFunc(partial.Providers, func(p config.PartialProviderSweep) bool { return p.Provider == "azure" }) + if idx < 0 { + partial.Providers = append(partial.Providers, config.PartialProviderSweep{Provider: "azure", Reasons: map[string]int{}}) + idx = len(partial.Providers) - 1 + } + sweep := &partial.Providers[idx] + if sweep.Reasons == nil { + sweep.Reasons = map[string]int{} + } + if sweep.Reasons[config.PartialReasonPricingUnavailable] == 0 { + sweep.Reasons[config.PartialReasonPricingUnavailable] = 1 + } + held := slices.Concat(sweep.HeldAccountIDs, carried) + slices.Sort(held) + sweep.HeldAccountIDs = slices.Compact(held) + sweep.IncompleteAccounts = max(sweep.IncompleteAccounts, len(sweep.HeldAccountIDs)) + return partial +} + +// previousAzureHold returns the Azure entry that records a pricing hold, or nil. +func previousAzureHold(partial *config.RecommendationsCollectionPartial) *config.PartialProviderSweep { + for i := range partial.Providers { + sweep := &partial.Providers[i] + if sweep.Provider == "azure" && sweep.Reasons[config.PartialReasonPricingUnavailable] > 0 { + return sweep + } + } + return nil +} + +// stillHeld returns the previously held accounts this cycle did not re-price, +// and whether an account-less (ambient) hold is still unresolved. +func stillHeld(prev *config.PartialProviderSweep, collects []config.SuccessfulCollect) (carried []string, unresolved bool) { + repriced := map[string]bool{} + ambientRepriced := false + for _, sc := range collects { + switch { + case sc.Provider != "azure": + case sc.CloudAccountID == nil: + ambientRepriced = true + default: + repriced[*sc.CloudAccountID] = true + } + } + for _, id := range prev.HeldAccountIDs { + if !repriced[id] { + carried = append(carried, id) + } + } + return carried, len(prev.HeldAccountIDs) == 0 && !ambientRepriced +} diff --git a/internal/scheduler/azure_hold_carry_integration_test.go b/internal/scheduler/azure_hold_carry_integration_test.go new file mode 100644 index 000000000..40d2fdd96 --- /dev/null +++ b/internal/scheduler/azure_hold_carry_integration_test.go @@ -0,0 +1,105 @@ +//go:build integration + +package scheduler + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +func heldPartial(ids ...string) *config.RecommendationsCollectionPartial { + return &config.RecommendationsCollectionPartial{Providers: []config.PartialProviderSweep{{ + Provider: "azure", IncompleteAccounts: len(ids), HeldAccountIDs: ids, + Reasons: map[string]int{config.PartialReasonPricingUnavailable: 1}, + }}} +} + +func heldAzureAccounts(t *testing.T, store config.StoreInterface) []string { + t.Helper() + fresh, err := store.GetRecommendationsFreshness(context.Background()) + require.NoError(t, err) + if fresh.LastCollectionPartial == nil { + return nil + } + for _, sweep := range fresh.LastCollectionPartial.Providers { + if sweep.Provider == "azure" && sweep.Reasons[config.PartialReasonPricingUnavailable] > 0 { + return sweep.HeldAccountIDs + } + } + return nil +} + +// The hold marker lasts as long as the held rows: it survives every cycle that +// does not re-price the account, and is dropped only by a cycle that does. +func TestAzureHoldCarriedAcrossCycles(t *testing.T) { + ctx := context.Background() + acctA, acctB := uuid.NewString(), uuid.NewString() + newScheduler := func(t *testing.T) (*Scheduler, config.StoreInterface) { + container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(container.DB) + s := &Scheduler{config: store} + s.persistCollection(ctx, nil, nil, nil, heldPartial(acctA)) + require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) + return s, store + } + azureOK := func(id string) []config.SuccessfulCollect { return expandSuccessfulCollects("azure", []string{id}) } + + t.Run("outright Azure failure keeps the hold", func(t *testing.T) { + s, store := newScheduler(t) + s.persistCollection(ctx, nil, nil, map[string]string{"azure": "boom"}, nil) + require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) + }) + + t.Run("run without Azure keeps the hold", func(t *testing.T) { + s, store := newScheduler(t) + s.persistCollection(ctx, nil, expandSuccessfulCollects("aws", []string{""}), nil, nil) + require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) + }) + + t.Run("an outage on another account holds both", func(t *testing.T) { + s, store := newScheduler(t) + s.persistCollection(ctx, nil, nil, nil, heldPartial(acctB)) + require.ElementsMatch(t, []string{acctA, acctB}, heldAzureAccounts(t, store)) + }) + + t.Run("a good cycle for another account keeps this hold", func(t *testing.T) { + s, store := newScheduler(t) + s.persistCollection(ctx, nil, azureOK(acctB), nil, nil) + require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) + }) + + t.Run("a good cycle for the account clears it", func(t *testing.T) { + s, store := newScheduler(t) + s.persistCollection(ctx, nil, azureOK(acctA), nil, nil) + require.Empty(t, heldAzureAccounts(t, store)) + }) + + t.Run("a good cycle re-prices one of two held accounts", func(t *testing.T) { + s, store := newScheduler(t) + s.persistCollection(ctx, nil, nil, nil, heldPartial(acctB)) + s.persistCollection(ctx, nil, azureOK(acctA), nil, nil) + require.Equal(t, []string{acctB}, heldAzureAccounts(t, store)) + }) + + t.Run("an unresolved ambient hold survives an outright failure", func(t *testing.T) { + container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(container.DB) + s := &Scheduler{config: store} + collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips}) + s.persistCollection(ctx, nil, nil, map[string]string{"azure": "boom"}, nil) + fresh, err := store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.NotNil(t, fresh.LastCollectionPartial, "no registered account to name, but the hold must remain") + collectAzureAmbientCycle(t, store, []common.Recommendation{partialSweepRec()}, nil) + fresh, err = store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.Nil(t, fresh.LastCollectionPartial) + }) +} diff --git a/internal/scheduler/azure_hold_carry_test.go b/internal/scheduler/azure_hold_carry_test.go new file mode 100644 index 000000000..9d7309d13 --- /dev/null +++ b/internal/scheduler/azure_hold_carry_test.go @@ -0,0 +1,72 @@ +package scheduler + +import ( + "context" + "errors" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// If the previous hold cannot be read, nothing may be written: an upsert would +// reset the marker and re-open the purchase of held rows. Fail closed. +func TestPersistCollection_UnreadableHoldStateWritesNothing(t *testing.T) { + store := new(MockConfigStore) + upserted := false + store.On("GetRecommendationsFreshness", mock.Anything).Return(nil, errors.New("database unavailable")) + store.On("UpsertRecommendations", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Run(func(mock.Arguments) { upserted = true }).Return(nil).Maybe() + store.On("SetRecommendationsCollectionError", mock.Anything, mock.Anything).Return(nil).Maybe() + s := &Scheduler{config: store} + + s.persistCollection(context.Background(), []config.RecommendationRecord{{ID: "r"}}, nil, nil, nil) + + assert.False(t, upserted, "an upsert would clear the pricing hold marker") +} + +func TestCarryForwardAzureHold(t *testing.T) { + prev := heldPrevious("a", "b") + for name, tc := range map[string]struct { + collects []config.SuccessfulCollect + partial *config.RecommendationsCollectionPartial + want []string + }{ + "nothing re-priced": {nil, nil, []string{"a", "b"}}, + "one re-priced": {expandSuccessfulCollects("azure", []string{"a"}), nil, []string{"b"}}, + "both re-priced": {expandSuccessfulCollects("azure", []string{"a", "b"}), nil, nil}, + "other provider ignored": {expandSuccessfulCollects("gcp", []string{"a"}), nil, []string{"a", "b"}}, + "new hold is merged": {nil, heldPartialOf("c"), []string{"a", "b", "c"}}, + "re-held account dedupes": {nil, heldPartialOf("a"), []string{"a", "b"}}, + } { + t.Run(name, func(t *testing.T) { + store := new(MockConfigStore) + store.On("GetRecommendationsFreshness", mock.Anything).Return(prev, nil) + s := &Scheduler{config: store} + + got, err := s.carryForwardAzureHold(context.Background(), tc.partial, tc.collects) + + require.NoError(t, err) + if tc.want == nil { + assert.Nil(t, got) + return + } + require.NotNil(t, got) + require.Len(t, got.Providers, 1) + assert.ElementsMatch(t, tc.want, got.Providers[0].HeldAccountIDs) + assert.Positive(t, got.Providers[0].Reasons[config.PartialReasonPricingUnavailable]) + }) + } +} + +func heldPartialOf(ids ...string) *config.RecommendationsCollectionPartial { + return &config.RecommendationsCollectionPartial{Providers: []config.PartialProviderSweep{{ + Provider: "azure", HeldAccountIDs: ids, Reasons: map[string]int{config.PartialReasonPricingUnavailable: 1}, + }}} +} + +func heldPrevious(ids ...string) *config.RecommendationsFreshness { + return &config.RecommendationsFreshness{LastCollectionPartial: heldPartialOf(ids...)} +} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 8d4f48a8c..edfd7c230 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -445,6 +445,15 @@ func (s *Scheduler) clearCollectionStartedBestEffort(ctx context.Context, ownerT // stops showing instead of staying forever (issue #120). A failed write skips // the prune. func (s *Scheduler) persistCollection(ctx context.Context, recs []config.RecommendationRecord, successfulCollects []config.SuccessfulCollect, failedProviders map[string]string, partial *config.RecommendationsCollectionPartial) { + partial, err := s.carryForwardAzureHold(ctx, partial, successfulCollects) + if err != nil { + // Upserting would reset the hold marker for rows that are still held. + logging.Errorf("Not persisting recommendations: %v", err) + if setErr := s.config.SetRecommendationsCollectionError(ctx, err.Error()); setErr != nil { + logging.Errorf("Failed to record collection error: %v", setErr) + } + return + } if err := s.config.UpsertRecommendations(ctx, time.Now().UTC(), recs, successfulCollects); err != nil { logging.Errorf("Failed to persist recommendations: %v", err) if setErr := s.config.SetRecommendationsCollectionError(ctx, err.Error()); setErr != nil { From af1f690da499f9dd995cb89972dce1c6fbc21639 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 06:12:50 +0100 Subject: [PATCH 14/27] fix(config): write the partial-sweep marker in the upsert transaction UpsertRecommendations committed and cleared last_collection_partial, and the scheduler rewrote it in a second step. If that write failed, the Azure pricing hold was lost while the held rows stayed purchasable. Add UpsertRecommendationsWithPartial, which writes the rows and the marker in one transaction, and use it for partial collections. Co-Authored-By: Claude Opus 5.5 --- internal/config/interfaces.go | 4 +++ .../config/store_postgres_recommendations.go | 21 ++++++++++-- internal/mocks/stores.go | 9 +++++ .../azure_hold_carry_integration_test.go | 34 +++++++++++++++++++ internal/scheduler/partial_sweep_test.go | 18 +++++----- internal/scheduler/scheduler.go | 27 ++++++++------- .../scheduler/stale_recommendations_test.go | 3 +- 7 files changed, 92 insertions(+), 24 deletions(-) diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 958c6227a..b386e4488 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -692,6 +692,10 @@ type StoreInterface interface { // SuccessfulCollect for the per-row semantics. There is deliberately no // unscoped "replace everything" method (issue #159). UpsertRecommendations(ctx context.Context, collectedAt time.Time, recs []RecommendationRecord, successfulCollects []SuccessfulCollect) error + // UpsertRecommendationsWithPartial is UpsertRecommendations that also + // records the collection's incomplete-sweep summary (nil clears it) in the + // same transaction, so the rows and the marker commit or roll back together. + UpsertRecommendationsWithPartial(ctx context.Context, collectedAt time.Time, recs []RecommendationRecord, successfulCollects []SuccessfulCollect, partial *RecommendationsCollectionPartial) error // PruneStaleRecommendations deletes the rows of every (provider, // account) pair with no row collected in the last retentionDays days, // leaving every pair that collected inside the window untouched, and diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index 5987c3fb4..afdb52104 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -45,6 +45,23 @@ const azurePricingMigrationVersion = 106 // insert. This collapses ambient and registered identities consistently // on both sides of the join. func (s *PostgresStore) UpsertRecommendations(ctx context.Context, collectedAt time.Time, recs []RecommendationRecord, successfulCollects []SuccessfulCollect) error { + return s.UpsertRecommendationsWithPartial(ctx, collectedAt, recs, successfulCollects, nil) +} + +// UpsertRecommendationsWithPartial is UpsertRecommendations that records the +// collection's incomplete-sweep summary in the same transaction (nil clears +// it). Writing the marker separately left a window in which a failed second +// write dropped the Azure pricing hold while the held rows were already +// committed. +func (s *PostgresStore) UpsertRecommendationsWithPartial(ctx context.Context, collectedAt time.Time, recs []RecommendationRecord, successfulCollects []SuccessfulCollect, partial *RecommendationsCollectionPartial) error { + var partialJSON []byte + if partial != nil { + encoded, err := json.Marshal(partial) + if err != nil { + return fmt.Errorf("failed to encode collection partial: %w", err) + } + partialJSON = encoded + } tx, err := s.db.Begin(ctx) if err != nil { return fmt.Errorf("failed to begin tx: %w", err) @@ -83,9 +100,9 @@ func (s *PostgresStore) UpsertRecommendations(ctx context.Context, collectedAt t UPDATE recommendations_state SET last_collected_at = GREATEST(last_collected_at, $1), last_collection_error = NULL, - last_collection_partial = NULL + last_collection_partial = $2::jsonb WHERE id = 1 - `, collectedAt); err != nil { + `, collectedAt, partialJSON); err != nil { return fmt.Errorf("failed to update state: %w", err) } diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index afcc07417..1d86a2dab 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -1876,6 +1876,15 @@ func (m *MockConfigStore) UpsertRecommendations(ctx context.Context, collectedAt return args.Error(0) } +func (m *MockConfigStore) UpsertRecommendationsWithPartial(ctx context.Context, collectedAt time.Time, recs []config.RecommendationRecord, successfulCollects []config.SuccessfulCollect, partial *config.RecommendationsCollectionPartial) error { + m.record("UpsertRecommendationsWithPartial", ctx, collectedAt, recs, successfulCollects, partial) + if !isExpected(&m.Mock, "UpsertRecommendationsWithPartial") { + return nil + } + args := m.Called(ctx, collectedAt, recs, successfulCollects, partial) + return args.Error(0) +} + func (m *MockConfigStore) PruneStaleRecommendations(ctx context.Context, retentionDays int) (int64, error) { m.record("PruneStaleRecommendations", ctx, retentionDays) if !isExpected(&m.Mock, "PruneStaleRecommendations") { diff --git a/internal/scheduler/azure_hold_carry_integration_test.go b/internal/scheduler/azure_hold_carry_integration_test.go index 40d2fdd96..ec35ae3eb 100644 --- a/internal/scheduler/azure_hold_carry_integration_test.go +++ b/internal/scheduler/azure_hold_carry_integration_test.go @@ -103,3 +103,37 @@ func TestAzureHoldCarriedAcrossCycles(t *testing.T) { require.Nil(t, fresh.LastCollectionPartial) }) } + +// The rows and the hold marker are written in one transaction: when writing the +// new marker fails, the upsert rolls back too, so the old rows and the old hold +// both stay. Before, the upsert committed and reset the marker first, and a +// failing second write left the held rows purchasable. +func TestPersistCollection_FailedMarkerWriteRollsBackUpsert(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(container.DB) + s := &Scheduler{config: store} + acctA := uuid.NewString() + seedAdvisorRow(t, store) + s.persistCollection(ctx, nil, nil, nil, heldPartial(acctA)) + require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) + + _, err := container.DB.Exec(ctx, ` + CREATE FUNCTION fail_poisoned_marker() RETURNS trigger AS $$ + BEGIN + IF NEW.last_collection_partial::text LIKE '%poison%' THEN + RAISE EXCEPTION 'injected marker write failure'; + END IF; + RETURN NEW; + END $$ LANGUAGE plpgsql; + CREATE TRIGGER fail_poisoned_marker BEFORE UPDATE ON recommendations_state + FOR EACH ROW EXECUTE FUNCTION fail_poisoned_marker();`) + require.NoError(t, err) + + newRow := config.RecommendationRecord{ID: "new-row", Provider: "azure", Service: "compute", Region: "eastus", ResourceType: "Standard_D4s_v3", + Savings: 10, Count: 1, Term: 12, Payment: "upfront"} + s.persistCollection(ctx, []config.RecommendationRecord{newRow}, nil, nil, heldPartial("poison")) + + require.Equal(t, []string{"prev-advisor-row"}, storedAzureIDs(t, store), "the upsert must roll back with the marker") + require.Equal(t, []string{acctA}, heldAzureAccounts(t, store), "the old hold must survive a failed marker write") +} diff --git a/internal/scheduler/partial_sweep_test.go b/internal/scheduler/partial_sweep_test.go index 4448a52a9..fd8938ab6 100644 --- a/internal/scheduler/partial_sweep_test.go +++ b/internal/scheduler/partial_sweep_test.go @@ -189,15 +189,17 @@ func expectScheduledCollection(store *MockConfigStore, providers ...string) (*[] store.On("UpsertRecommendations", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Run(func(args mock.Arguments) { upserted = args.Get(2).([]config.RecommendationRecord) - }).Return(nil).Once() + }).Return(nil).Maybe() - // Registered as Maybe so an unwanted call is RECORDED: MockConfigStore - // short-circuits unexpected methods before mock.Called, which would - // make AssertNotCalled pass vacuously. + // A partial collection writes its marker in the same transaction as the + // rows. Registered as Maybe so an unwanted call is RECORDED: + // MockConfigStore short-circuits unexpected methods before mock.Called, + // which would make AssertNotCalled pass vacuously. var recorded *config.RecommendationsCollectionPartial - store.On("SetRecommendationsCollectionPartial", mock.Anything, mock.Anything). + store.On("UpsertRecommendationsWithPartial", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). Run(func(args mock.Arguments) { - recorded = args.Get(1).(*config.RecommendationsCollectionPartial) + upserted = args.Get(2).([]config.RecommendationRecord) + recorded = args.Get(4).(*config.RecommendationsCollectionPartial) }).Return(nil).Maybe() return &upserted, &recorded } @@ -241,7 +243,7 @@ func TestCollectRecommendations_CompleteSweepLeavesPartialCleared(t *testing.T) require.NoError(t, err) assert.Len(t, *upserted, 1) - store.AssertNotCalled(t, "SetRecommendationsCollectionPartial", mock.Anything, mock.Anything) + store.AssertNotCalled(t, "UpsertRecommendationsWithPartial", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // TestCollectRecommendations_TotalFailureUnchanged: a hard provider failure @@ -260,7 +262,7 @@ func TestCollectRecommendations_TotalFailureUnchanged(t *testing.T) { require.NoError(t, err) assert.Contains(t, result.FailedProviders, "azure") assert.Contains(t, recordedErr, "azure: ") - store.AssertNotCalled(t, "SetRecommendationsCollectionPartial", mock.Anything, mock.Anything) + store.AssertNotCalled(t, "UpsertRecommendationsWithPartial", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // A partial provider and a complete one: only the partial provider appears. diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index edfd7c230..908eb1d3c 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -433,11 +433,10 @@ func (s *Scheduler) clearCollectionStartedBestEffort(ctx context.Context, ownerT // recorded in recommendations_state so the frontend banner renders while // the user still sees the valid rows we managed to upsert. // -// UpsertRecommendations also resets last_collection_partial, so a complete -// collection clears the previous run's partial marker; a non-nil partial -// re-records it straight after (issue #87). The two writes mirror the -// existing error-column pattern and are not atomic: if the second write -// fails, the run is logged and reads as complete until the next collection. +// The rows and last_collection_partial commit in one transaction: a complete +// collection clears the previous run's marker, a non-nil partial replaces it +// (issue #87). A failed marker write rolls the rows back too, so the Azure +// pricing hold cannot be lost while the held rows stay. // // After a successful write it prunes the rows of (provider, account) pairs // that have not collected for CUDLY_STALE_RECOMMENDATION_RETENTION_DAYS @@ -454,7 +453,7 @@ func (s *Scheduler) persistCollection(ctx context.Context, recs []config.Recomme } return } - if err := s.config.UpsertRecommendations(ctx, time.Now().UTC(), recs, successfulCollects); err != nil { + if err := s.upsertWithMarker(ctx, recs, successfulCollects, partial); err != nil { logging.Errorf("Failed to persist recommendations: %v", err) if setErr := s.config.SetRecommendationsCollectionError(ctx, err.Error()); setErr != nil { logging.Errorf("Failed to record write error: %v", setErr) @@ -468,15 +467,19 @@ func (s *Scheduler) persistCollection(ctx context.Context, recs []config.Recomme } } - if partial != nil { - if err := s.config.SetRecommendationsCollectionPartial(ctx, partial); err != nil { - logging.Errorf("Failed to record partial collection: %v", err) - } - } - s.pruneStaleRecommendationsBestEffort(ctx) } +// upsertWithMarker writes the rows and the collection's partial marker in one +// transaction. A complete collection (nil partial) needs only +// UpsertRecommendations, which clears the marker in its own transaction. +func (s *Scheduler) upsertWithMarker(ctx context.Context, recs []config.RecommendationRecord, successfulCollects []config.SuccessfulCollect, partial *config.RecommendationsCollectionPartial) error { + if partial == nil { + return s.config.UpsertRecommendations(ctx, time.Now().UTC(), recs, successfulCollects) + } + return s.config.UpsertRecommendationsWithPartial(ctx, time.Now().UTC(), recs, successfulCollects, partial) +} + // joinProviderErrors formats a "provider: err; provider: err" string for // the state table's last_collection_error column. func joinProviderErrors(failed map[string]string) string { diff --git a/internal/scheduler/stale_recommendations_test.go b/internal/scheduler/stale_recommendations_test.go index 11dd09a23..5f43903ee 100644 --- a/internal/scheduler/stale_recommendations_test.go +++ b/internal/scheduler/stale_recommendations_test.go @@ -79,8 +79,7 @@ func TestScheduler_persistCollection_PruneErrorIsBestEffort(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) partial := &config.RecommendationsCollectionPartial{} - mockStore.On("UpsertRecommendations", ctx, mock.Anything, mock.Anything, mock.Anything).Return(nil) - mockStore.On("SetRecommendationsCollectionPartial", ctx, partial).Return(nil) + mockStore.On("UpsertRecommendationsWithPartial", ctx, mock.Anything, mock.Anything, mock.Anything, partial).Return(nil) mockStore.On("PruneStaleRecommendations", ctx, config.DefaultStaleRecommendationRetentionDays). Return(int64(0), errors.New("lock timeout")) From 7e9d3526ef68bda449c1937b310a849975f3d1d7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 06:41:13 +0100 Subject: [PATCH 15/27] fix(config): merge the Azure pricing hold inside the upsert transaction The hold carry-over read the previous marker outside the transaction and the upsert then overwrote it unconditionally, so a cron run and a manual refresh that overlapped could drop each other's hold while the held rows stayed purchasable. The store now locks the state row (FOR UPDATE) and merges the previous hold into the new marker in the same transaction; an unreadable row or account list fails the tx and writes nothing. The merge also drops held accounts that are no longer enabled Azure accounts (deleted or disabled ones are never re-priced) and clears an ambient hold once registered accounts exist. The scheduler's out-of-tx read path is deleted, and SetRecommendationsCollectionPartial, which has no caller left, is removed. Co-Authored-By: Claude Opus 5.5 --- internal/config/azure_hold_carry.go | 92 +++++++++++++++++ internal/config/interfaces.go | 5 - .../config/store_postgres_recommendations.go | 99 +++++++++++-------- .../store_postgres_recommendations_test.go | 8 +- internal/mocks/stores.go | 9 -- internal/scheduler/azure_hold_carry.go | 97 ------------------ internal/scheduler/azure_hold_carry_test.go | 72 -------------- internal/scheduler/scheduler.go | 13 +-- 8 files changed, 155 insertions(+), 240 deletions(-) create mode 100644 internal/config/azure_hold_carry.go delete mode 100644 internal/scheduler/azure_hold_carry.go delete mode 100644 internal/scheduler/azure_hold_carry_test.go diff --git a/internal/config/azure_hold_carry.go b/internal/config/azure_hold_carry.go new file mode 100644 index 000000000..af0851c46 --- /dev/null +++ b/internal/config/azure_hold_carry.go @@ -0,0 +1,92 @@ +package config + +import ( + "slices" +) + +// carryAzureHold keeps the Azure pricing hold alive for as long as the held +// rows are. Every upsert replaces last_collection_partial, but an account's +// Advisor rows stay at their old price until a later cycle re-prices them, so +// the previous hold (prev, read under the state row lock) is merged into this +// cycle's partial: accounts re-priced now (in collects) drop out, newly held +// ones are added, the rest carry over. That covers another outage, an outright +// Azure failure and a run that skips Azure. +// +// A held account that is no longer an enabled registered Azure account (enabled +// holds its IDs) is dropped, since it will never be collected again. A hold +// with no account IDs (an unresolved ambient identity) carries until an +// ambient Azure collection succeeds or registered Azure accounts exist, after +// which the ambient identity is no longer collected. +func carryAzureHold(prev, partial *RecommendationsCollectionPartial, collects []SuccessfulCollect, enabled map[string]bool) *RecommendationsCollectionPartial { + held := previousAzureHold(prev) + if held == nil { + return partial + } + carried, unresolved := stillHeld(held, collects, enabled) + if len(carried) == 0 && !unresolved { + return partial + } + return mergeCarriedHold(partial, carried) +} + +// mergeCarriedHold adds the carried account IDs and the pricing_unavailable +// reason to partial's Azure entry, creating the entry when this cycle had none. +func mergeCarriedHold(partial *RecommendationsCollectionPartial, carried []string) *RecommendationsCollectionPartial { + if partial == nil { + partial = &RecommendationsCollectionPartial{} + } + idx := slices.IndexFunc(partial.Providers, func(p PartialProviderSweep) bool { return p.Provider == "azure" }) + if idx < 0 { + partial.Providers = append(partial.Providers, PartialProviderSweep{Provider: "azure", Reasons: map[string]int{}}) + idx = len(partial.Providers) - 1 + } + sweep := &partial.Providers[idx] + if sweep.Reasons == nil { + sweep.Reasons = map[string]int{} + } + if sweep.Reasons[PartialReasonPricingUnavailable] == 0 { + sweep.Reasons[PartialReasonPricingUnavailable] = 1 + } + held := slices.Concat(sweep.HeldAccountIDs, carried) + slices.Sort(held) + sweep.HeldAccountIDs = slices.Compact(held) + sweep.IncompleteAccounts = max(sweep.IncompleteAccounts, len(sweep.HeldAccountIDs)) + return partial +} + +// previousAzureHold returns the Azure entry that records a pricing hold, or nil. +func previousAzureHold(partial *RecommendationsCollectionPartial) *PartialProviderSweep { + if partial == nil { + return nil + } + for i := range partial.Providers { + sweep := &partial.Providers[i] + if sweep.Provider == "azure" && sweep.Reasons[PartialReasonPricingUnavailable] > 0 { + return sweep + } + } + return nil +} + +// stillHeld returns the previously held accounts this cycle did not re-price, +// that are still enabled, and whether an account-less (ambient) hold is still +// unresolved. +func stillHeld(prev *PartialProviderSweep, collects []SuccessfulCollect, enabled map[string]bool) (carried []string, unresolved bool) { + repriced := map[string]bool{} + ambientRepriced := false + for _, sc := range collects { + switch { + case sc.Provider != "azure": + case sc.CloudAccountID == nil: + ambientRepriced = true + default: + repriced[*sc.CloudAccountID] = true + } + } + for _, id := range prev.HeldAccountIDs { + if !repriced[id] && enabled[id] { + carried = append(carried, id) + } + } + return carried, len(prev.HeldAccountIDs) == 0 && !ambientRepriced && len(enabled) == 0 +} diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index b386e4488..482b5a42f 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -716,11 +716,6 @@ type StoreInterface interface { PageStoredRecommendations(ctx context.Context, q RecommendationPageQuery) (RecommendationPage, error) GetRecommendationsFreshness(ctx context.Context) (*RecommendationsFreshness, error) SetRecommendationsCollectionError(ctx context.Context, errMsg string) error - // SetRecommendationsCollectionPartial records (or, with nil, clears) the - // most recent collection's incomplete-sweep summary. - // UpsertRecommendations clears it, so a later complete collection drops - // the marker. - SetRecommendationsCollectionPartial(ctx context.Context, partial *RecommendationsCollectionPartial) error // MarkCollectionStarted atomically sets last_collection_started_at = now // only when no in-flight collection is running (last_collection_started_at IS NULL // OR older than 5 minutes), and stamps a freshly generated owner token diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index afdb52104..853947fd6 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -49,19 +49,11 @@ func (s *PostgresStore) UpsertRecommendations(ctx context.Context, collectedAt t } // UpsertRecommendationsWithPartial is UpsertRecommendations that records the -// collection's incomplete-sweep summary in the same transaction (nil clears -// it). Writing the marker separately left a window in which a failed second -// write dropped the Azure pricing hold while the held rows were already -// committed. +// collection's incomplete-sweep summary in the same transaction. The previous +// Azure pricing hold is read under a row lock and merged into partial (see +// carryAzureHold), so overlapping collections cannot drop each other's hold +// and a failed read or write rolls the rows back with the marker. func (s *PostgresStore) UpsertRecommendationsWithPartial(ctx context.Context, collectedAt time.Time, recs []RecommendationRecord, successfulCollects []SuccessfulCollect, partial *RecommendationsCollectionPartial) error { - var partialJSON []byte - if partial != nil { - encoded, err := json.Marshal(partial) - if err != nil { - return fmt.Errorf("failed to encode collection partial: %w", err) - } - partialJSON = encoded - } tx, err := s.db.Begin(ctx) if err != nil { return fmt.Errorf("failed to begin tx: %w", err) @@ -89,6 +81,17 @@ func (s *PostgresStore) UpsertRecommendationsWithPartial(ctx context.Context, co } } + // The state row lock is taken last, after the row writes, so the lock order + // is unchanged from the plain UPDATE this replaced. + partial, err = carryHoldInTx(ctx, tx, partial, successfulCollects) + if err != nil { + return err + } + partialJSON, err := encodeCollectionPartial(partial) + if err != nil { + return err + } + // last_collected_at only moves forward (issue #111). Two collections can // overlap (cron and a user refresh), and the one that started first can // commit last; an unconditional SET would then move freshness backwards @@ -112,6 +115,50 @@ func (s *PostgresStore) UpsertRecommendationsWithPartial(ctx context.Context, co return nil } +// encodeCollectionPartial marshals the marker for the JSONB column; nil stays +// nil so the column is cleared. +func encodeCollectionPartial(partial *RecommendationsCollectionPartial) ([]byte, error) { + if partial == nil { + return nil, nil + } + encoded, err := json.Marshal(partial) + if err != nil { + return nil, fmt.Errorf("failed to encode collection partial: %w", err) + } + return encoded, nil +} + +// carryHoldInTx locks the state row, then merges the previous Azure pricing +// hold into partial. The lock serializes this read-merge-write against every +// concurrent collection. An unreadable row or account list fails the tx. +func carryHoldInTx(ctx context.Context, tx pgx.Tx, partial *RecommendationsCollectionPartial, collects []SuccessfulCollect) (*RecommendationsCollectionPartial, error) { + var raw []byte + if err := tx.QueryRow(ctx, `SELECT last_collection_partial FROM recommendations_state WHERE id = 1 FOR UPDATE`).Scan(&raw); err != nil { + return nil, fmt.Errorf("failed to lock recommendations_state: %w", err) + } + prev := decodeCollectionPartial(raw) + if previousAzureHold(prev) == nil { + return partial, nil + } + rows, err := tx.Query(ctx, `SELECT id::text FROM cloud_accounts WHERE provider = 'azure' AND enabled`) + if err != nil { + return nil, fmt.Errorf("failed to list enabled azure accounts: %w", err) + } + defer rows.Close() + enabled := map[string]bool{} + for rows.Next() { + var id string + if err := rows.Scan(&id); err != nil { + return nil, fmt.Errorf("failed to scan azure account id: %w", err) + } + enabled[id] = true + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("failed to list enabled azure accounts: %w", err) + } + return carryAzureHold(prev, partial, collects, enabled), nil +} + // successfulCollectArrays converts the SuccessfulCollect slice into the two // parallel arrays the eviction unnest() expects. nil CloudAccountID becomes // uuid.Nil so the join matches the generated account_key column for @@ -451,7 +498,7 @@ func (s *PostgresStore) GetRecommendationsFreshness(ctx context.Context) (*Recom // decodeCollectionPartial decodes the last_collection_partial JSONB column. // NULL decodes to nil (the last collection was complete). The column is only -// ever written by SetRecommendationsCollectionPartial via json.Marshal, so an +// ever written by UpsertRecommendationsWithPartial via json.Marshal, so an // undecodable value means the row was edited by hand; it is logged and read // as nil rather than failing the whole freshness read, which the scheduler's // cache-TTL check and every dashboard load depend on. @@ -493,32 +540,6 @@ func (s *PostgresStore) SetRecommendationsCollectionError(ctx context.Context, e return nil } -// SetRecommendationsCollectionPartial records the most recent collection's -// incomplete-sweep summary (issue #87), or clears it when partial is nil. -// Like SetRecommendationsCollectionError it is called by the scheduler AFTER -// UpsertRecommendations has reset the column to NULL, so a complete -// collection leaves it cleared and only a partial one re-sets it. It touches -// no other column; in particular it must not clear last_collection_started_at -// (see SetRecommendationsCollectionError, issue #261). -func (s *PostgresStore) SetRecommendationsCollectionPartial(ctx context.Context, partial *RecommendationsCollectionPartial) error { - var raw []byte - if partial != nil { - encoded, err := json.Marshal(partial) - if err != nil { - return fmt.Errorf("failed to encode collection partial: %w", err) - } - raw = encoded - } - if _, err := s.db.Exec(ctx, ` - UPDATE recommendations_state - SET last_collection_partial = $1::jsonb - WHERE id = 1 - `, raw); err != nil { - return fmt.Errorf("failed to set collection partial: %w", err) - } - return nil -} - // MarkCollectionStarted atomically sets last_collection_started_at = NOW() // only when no in-flight collection is currently running. The WHERE clause // treats a started_at older than 5 minutes as stale (the scheduler Lambda diff --git a/internal/config/store_postgres_recommendations_test.go b/internal/config/store_postgres_recommendations_test.go index 493da93aa..c7c4c74d3 100644 --- a/internal/config/store_postgres_recommendations_test.go +++ b/internal/config/store_postgres_recommendations_test.go @@ -225,7 +225,7 @@ func TestPostgresStore_CollectionPartial_RecordedAndCleared(t *testing.T) { } recordPartial := func() { t.Helper() - require.NoError(t, store.SetRecommendationsCollectionPartial(ctx, partial)) + require.NoError(t, store.UpsertRecommendationsWithPartial(ctx, time.Now().UTC(), nil, nil, partial)) got, getErr := store.GetRecommendationsFreshness(ctx) require.NoError(t, getErr) require.Equal(t, partial, got.LastCollectionPartial) @@ -243,12 +243,6 @@ func TestPostgresStore_CollectionPartial_RecordedAndCleared(t *testing.T) { require.NoError(t, err) assert.Nil(t, fr.LastCollectionPartial, "the next persisted collection must clear the marker") assert.Nil(t, fr.LastCollectionError) - - recordPartial() - require.NoError(t, store.SetRecommendationsCollectionPartial(ctx, nil)) - fr, err = store.GetRecommendationsFreshness(ctx) - require.NoError(t, err) - assert.Nil(t, fr.LastCollectionPartial, "a nil partial clears the column") } // TestPostgresStore_UpsertRecommendations_StoresAllTermVariants pins diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 1d86a2dab..0c9db1eb9 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -1983,15 +1983,6 @@ func (m *MockConfigStore) SetRecommendationsCollectionError(ctx context.Context, return args.Error(0) } -func (m *MockConfigStore) SetRecommendationsCollectionPartial(ctx context.Context, partial *config.RecommendationsCollectionPartial) error { - m.record("SetRecommendationsCollectionPartial", ctx, partial) - if !isExpected(&m.Mock, "SetRecommendationsCollectionPartial") { - return nil - } - args := m.Called(ctx, partial) - return args.Error(0) -} - func (m *MockConfigStore) GetRIUtilizationCache(ctx context.Context, key config.RIUtilizationCacheKey) (*config.RIUtilizationCacheEntry, error) { m.record("GetRIUtilizationCache", ctx, key) if !isExpected(&m.Mock, "GetRIUtilizationCache") { diff --git a/internal/scheduler/azure_hold_carry.go b/internal/scheduler/azure_hold_carry.go deleted file mode 100644 index 6626579aa..000000000 --- a/internal/scheduler/azure_hold_carry.go +++ /dev/null @@ -1,97 +0,0 @@ -package scheduler - -import ( - "context" - "fmt" - "slices" - - "github.com/LeanerCloud/cloud-commitments-platform/internal/config" -) - -// carryForwardAzureHold keeps the Azure pricing hold alive for as long as the -// held rows are. UpsertRecommendations resets last_collection_partial on every -// cycle, but an account's Advisor rows stay at their old price until a later -// cycle re-prices them, so the previous hold is merged into this cycle's state: -// accounts re-priced now (collected with no outage skips, hence in collects) -// drop out, newly held ones are added, the rest carry over. That covers -// another outage, an outright Azure failure and a run that skips Azure. -// -// A hold with no account IDs (an unresolved ambient identity) carries until an -// ambient Azure collection succeeds. An error reading the previous state is -// returned: the caller must not upsert, because that would reset the marker. -func (s *Scheduler) carryForwardAzureHold(ctx context.Context, partial *config.RecommendationsCollectionPartial, collects []config.SuccessfulCollect) (*config.RecommendationsCollectionPartial, error) { - fresh, err := s.config.GetRecommendationsFreshness(ctx) - if err != nil { - return nil, fmt.Errorf("read previous pricing hold: %w", err) - } - if fresh == nil || fresh.LastCollectionPartial == nil { - return partial, nil - } - prev := previousAzureHold(fresh.LastCollectionPartial) - if prev == nil { - return partial, nil - } - carried, unresolved := stillHeld(prev, collects) - if len(carried) == 0 && !unresolved { - return partial, nil - } - return mergeCarriedHold(partial, carried), nil -} - -// mergeCarriedHold adds the carried account IDs and the pricing_unavailable -// reason to partial's Azure entry, creating the entry when this cycle had none. -func mergeCarriedHold(partial *config.RecommendationsCollectionPartial, carried []string) *config.RecommendationsCollectionPartial { - if partial == nil { - partial = &config.RecommendationsCollectionPartial{} - } - idx := slices.IndexFunc(partial.Providers, func(p config.PartialProviderSweep) bool { return p.Provider == "azure" }) - if idx < 0 { - partial.Providers = append(partial.Providers, config.PartialProviderSweep{Provider: "azure", Reasons: map[string]int{}}) - idx = len(partial.Providers) - 1 - } - sweep := &partial.Providers[idx] - if sweep.Reasons == nil { - sweep.Reasons = map[string]int{} - } - if sweep.Reasons[config.PartialReasonPricingUnavailable] == 0 { - sweep.Reasons[config.PartialReasonPricingUnavailable] = 1 - } - held := slices.Concat(sweep.HeldAccountIDs, carried) - slices.Sort(held) - sweep.HeldAccountIDs = slices.Compact(held) - sweep.IncompleteAccounts = max(sweep.IncompleteAccounts, len(sweep.HeldAccountIDs)) - return partial -} - -// previousAzureHold returns the Azure entry that records a pricing hold, or nil. -func previousAzureHold(partial *config.RecommendationsCollectionPartial) *config.PartialProviderSweep { - for i := range partial.Providers { - sweep := &partial.Providers[i] - if sweep.Provider == "azure" && sweep.Reasons[config.PartialReasonPricingUnavailable] > 0 { - return sweep - } - } - return nil -} - -// stillHeld returns the previously held accounts this cycle did not re-price, -// and whether an account-less (ambient) hold is still unresolved. -func stillHeld(prev *config.PartialProviderSweep, collects []config.SuccessfulCollect) (carried []string, unresolved bool) { - repriced := map[string]bool{} - ambientRepriced := false - for _, sc := range collects { - switch { - case sc.Provider != "azure": - case sc.CloudAccountID == nil: - ambientRepriced = true - default: - repriced[*sc.CloudAccountID] = true - } - } - for _, id := range prev.HeldAccountIDs { - if !repriced[id] { - carried = append(carried, id) - } - } - return carried, len(prev.HeldAccountIDs) == 0 && !ambientRepriced -} diff --git a/internal/scheduler/azure_hold_carry_test.go b/internal/scheduler/azure_hold_carry_test.go deleted file mode 100644 index 9d7309d13..000000000 --- a/internal/scheduler/azure_hold_carry_test.go +++ /dev/null @@ -1,72 +0,0 @@ -package scheduler - -import ( - "context" - "errors" - "testing" - - "github.com/LeanerCloud/cloud-commitments-platform/internal/config" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" -) - -// If the previous hold cannot be read, nothing may be written: an upsert would -// reset the marker and re-open the purchase of held rows. Fail closed. -func TestPersistCollection_UnreadableHoldStateWritesNothing(t *testing.T) { - store := new(MockConfigStore) - upserted := false - store.On("GetRecommendationsFreshness", mock.Anything).Return(nil, errors.New("database unavailable")) - store.On("UpsertRecommendations", mock.Anything, mock.Anything, mock.Anything, mock.Anything). - Run(func(mock.Arguments) { upserted = true }).Return(nil).Maybe() - store.On("SetRecommendationsCollectionError", mock.Anything, mock.Anything).Return(nil).Maybe() - s := &Scheduler{config: store} - - s.persistCollection(context.Background(), []config.RecommendationRecord{{ID: "r"}}, nil, nil, nil) - - assert.False(t, upserted, "an upsert would clear the pricing hold marker") -} - -func TestCarryForwardAzureHold(t *testing.T) { - prev := heldPrevious("a", "b") - for name, tc := range map[string]struct { - collects []config.SuccessfulCollect - partial *config.RecommendationsCollectionPartial - want []string - }{ - "nothing re-priced": {nil, nil, []string{"a", "b"}}, - "one re-priced": {expandSuccessfulCollects("azure", []string{"a"}), nil, []string{"b"}}, - "both re-priced": {expandSuccessfulCollects("azure", []string{"a", "b"}), nil, nil}, - "other provider ignored": {expandSuccessfulCollects("gcp", []string{"a"}), nil, []string{"a", "b"}}, - "new hold is merged": {nil, heldPartialOf("c"), []string{"a", "b", "c"}}, - "re-held account dedupes": {nil, heldPartialOf("a"), []string{"a", "b"}}, - } { - t.Run(name, func(t *testing.T) { - store := new(MockConfigStore) - store.On("GetRecommendationsFreshness", mock.Anything).Return(prev, nil) - s := &Scheduler{config: store} - - got, err := s.carryForwardAzureHold(context.Background(), tc.partial, tc.collects) - - require.NoError(t, err) - if tc.want == nil { - assert.Nil(t, got) - return - } - require.NotNil(t, got) - require.Len(t, got.Providers, 1) - assert.ElementsMatch(t, tc.want, got.Providers[0].HeldAccountIDs) - assert.Positive(t, got.Providers[0].Reasons[config.PartialReasonPricingUnavailable]) - }) - } -} - -func heldPartialOf(ids ...string) *config.RecommendationsCollectionPartial { - return &config.RecommendationsCollectionPartial{Providers: []config.PartialProviderSweep{{ - Provider: "azure", HeldAccountIDs: ids, Reasons: map[string]int{config.PartialReasonPricingUnavailable: 1}, - }}} -} - -func heldPrevious(ids ...string) *config.RecommendationsFreshness { - return &config.RecommendationsFreshness{LastCollectionPartial: heldPartialOf(ids...)} -} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 908eb1d3c..2956fe24f 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -444,15 +444,6 @@ func (s *Scheduler) clearCollectionStartedBestEffort(ctx context.Context, ownerT // stops showing instead of staying forever (issue #120). A failed write skips // the prune. func (s *Scheduler) persistCollection(ctx context.Context, recs []config.RecommendationRecord, successfulCollects []config.SuccessfulCollect, failedProviders map[string]string, partial *config.RecommendationsCollectionPartial) { - partial, err := s.carryForwardAzureHold(ctx, partial, successfulCollects) - if err != nil { - // Upserting would reset the hold marker for rows that are still held. - logging.Errorf("Not persisting recommendations: %v", err) - if setErr := s.config.SetRecommendationsCollectionError(ctx, err.Error()); setErr != nil { - logging.Errorf("Failed to record collection error: %v", setErr) - } - return - } if err := s.upsertWithMarker(ctx, recs, successfulCollects, partial); err != nil { logging.Errorf("Failed to persist recommendations: %v", err) if setErr := s.config.SetRecommendationsCollectionError(ctx, err.Error()); setErr != nil { @@ -471,8 +462,8 @@ func (s *Scheduler) persistCollection(ctx context.Context, recs []config.Recomme } // upsertWithMarker writes the rows and the collection's partial marker in one -// transaction. A complete collection (nil partial) needs only -// UpsertRecommendations, which clears the marker in its own transaction. +// transaction. Both store methods merge the previous Azure pricing hold under +// the state row lock, so a complete collection (nil partial) keeps it too. func (s *Scheduler) upsertWithMarker(ctx context.Context, recs []config.RecommendationRecord, successfulCollects []config.SuccessfulCollect, partial *config.RecommendationsCollectionPartial) error { if partial == nil { return s.config.UpsertRecommendations(ctx, time.Now().UTC(), recs, successfulCollects) From 792bb4c353f6653b6001b8455b7f760b05642d30 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 06:41:29 +0100 Subject: [PATCH 16/27] test(config): cover the in-transaction Azure hold merge Unit table for the merge (re-priced, deleted, disabled, ambient cases), a real-Postgres test where a later-committing collection that re-prices only Y must keep the hold another run committed on X (the second upsert is observed queued behind the first before it commits), and scheduler integration cases for deleted accounts, disabled accounts and an ambient hold that clears once accounts are registered. Co-Authored-By: Claude Opus 5.5 --- internal/config/azure_hold_carry_test.go | 57 ++++++++++++++++++ .../azure_hold_concurrent_integration_test.go | 59 +++++++++++++++++++ .../azure_hold_carry_integration_test.go | 44 +++++++++++++- 3 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 internal/config/azure_hold_carry_test.go create mode 100644 internal/config/azure_hold_concurrent_integration_test.go diff --git a/internal/config/azure_hold_carry_test.go b/internal/config/azure_hold_carry_test.go new file mode 100644 index 000000000..6e06f9053 --- /dev/null +++ b/internal/config/azure_hold_carry_test.go @@ -0,0 +1,57 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func holdOf(ids ...string) *RecommendationsCollectionPartial { + return &RecommendationsCollectionPartial{Providers: []PartialProviderSweep{{ + Provider: "azure", HeldAccountIDs: ids, Reasons: map[string]int{PartialReasonPricingUnavailable: 1}, + }}} +} + +func azureCollect(id *string) []SuccessfulCollect { + return []SuccessfulCollect{{Provider: "azure", CloudAccountID: id}} +} + +func TestCarryAzureHold(t *testing.T) { + a, b, c := "a", "b", "c" + enabled := map[string]bool{"a": true, "b": true, "c": true} + for name, tc := range map[string]struct { + prev *RecommendationsCollectionPartial + collects []SuccessfulCollect + partial *RecommendationsCollectionPartial + enabled map[string]bool + want []string + wantHold bool + }{ + "no previous hold": {nil, nil, nil, enabled, nil, false}, + "nothing re-priced": {holdOf("a", "b"), nil, nil, enabled, []string{"a", "b"}, true}, + "one re-priced": {holdOf("a", "b"), azureCollect(&a), nil, enabled, []string{"b"}, true}, + "both re-priced": {holdOf("a", "b"), append(azureCollect(&a), azureCollect(&b)...), nil, enabled, nil, false}, + "other provider ignored": {holdOf("a"), []SuccessfulCollect{{Provider: "gcp", CloudAccountID: &a}}, nil, enabled, []string{"a"}, true}, + "new hold is merged": {holdOf("a", "b"), nil, holdOf("c"), enabled, []string{"a", "b", "c"}, true}, + "re-held account dedupes": {holdOf("a", "b"), nil, holdOf("a"), enabled, []string{"a", "b"}, true}, + "deleted account drops": {holdOf("a", "b"), nil, nil, map[string]bool{"b": true}, []string{"b"}, true}, + "disabled account drops": {holdOf("a"), nil, nil, map[string]bool{"c": true}, nil, false}, + "ambient carries": {holdOf(), nil, nil, nil, nil, true}, + "ambient carries a fail": {holdOf(), azureCollect(&c), nil, nil, nil, true}, + "ambient re-priced": {holdOf(), azureCollect(nil), nil, nil, nil, false}, + "ambient clears once registered": {holdOf(), nil, nil, enabled, nil, false}, + } { + t.Run(name, func(t *testing.T) { + got := carryAzureHold(tc.prev, tc.partial, tc.collects, tc.enabled) + if !tc.wantHold { + assert.Nil(t, got) + return + } + require.NotNil(t, got) + require.Len(t, got.Providers, 1) + assert.ElementsMatch(t, tc.want, got.Providers[0].HeldAccountIDs) + assert.Positive(t, got.Providers[0].Reasons[PartialReasonPricingUnavailable]) + }) + } +} diff --git a/internal/config/azure_hold_concurrent_integration_test.go b/internal/config/azure_hold_concurrent_integration_test.go new file mode 100644 index 000000000..30ecbc596 --- /dev/null +++ b/internal/config/azure_hold_concurrent_integration_test.go @@ -0,0 +1,59 @@ +//go:build integration +// +build integration + +package config_test + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +// Two overlapping collections: one commits a pricing hold on X, the other +// (which started earlier and only re-prices Y) commits later. The later commit +// must not drop X's hold. The holder transaction stands in for the first +// collection mid-flight; the second upsert is provably queued behind it before +// the first commits. +func TestUpsertRecommendationsWithPartial_ConcurrentCollectionKeepsHold(t *testing.T) { + ctx := context.Background() + db := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()).DB + store := config.NewPostgresStore(db) + acctX, acctY := uuid.NewString(), uuid.NewString() + seedRecommendationCloudAccount(ctx, t, store, acctX, "azure", "sub-hold-x") + seedRecommendationCloudAccount(ctx, t, store, acctY, "azure", "sub-hold-y") + + holder, err := db.Begin(ctx) + require.NoError(t, err) + t.Cleanup(func() { _ = holder.Rollback(ctx) }) + _, err = holder.Exec(ctx, `SELECT 1 FROM recommendations_state WHERE id = 1 FOR UPDATE`) + require.NoError(t, err) + + done := make(chan error, 1) + go func() { + done <- store.UpsertRecommendationsWithPartial(ctx, time.Now().UTC(), nil, + []config.SuccessfulCollect{{Provider: "azure", CloudAccountID: &acctY}}, nil) + }() + _, finished := testhelpers.AwaitBlockedBy(ctx, t, db, testhelpers.BackendPID(ctx, t, holder), done) + require.False(t, finished, "the second collection must queue behind the first") + + hold, err := json.Marshal(&config.RecommendationsCollectionPartial{Providers: []config.PartialProviderSweep{{ + Provider: "azure", IncompleteAccounts: 1, HeldAccountIDs: []string{acctX}, + Reasons: map[string]int{config.PartialReasonPricingUnavailable: 1}, + }}}) + require.NoError(t, err) + _, err = holder.Exec(ctx, `UPDATE recommendations_state SET last_collection_partial = $1::jsonb WHERE id = 1`, hold) + require.NoError(t, err) + require.NoError(t, holder.Commit(ctx)) + require.NoError(t, testhelpers.Await(t, done)) + + fresh, err := store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.NotNil(t, fresh.LastCollectionPartial, "the later commit dropped the other run's hold") + require.Equal(t, []string{acctX}, fresh.LastCollectionPartial.Providers[0].HeldAccountIDs) +} diff --git a/internal/scheduler/azure_hold_carry_integration_test.go b/internal/scheduler/azure_hold_carry_integration_test.go index ec35ae3eb..3d0f7adec 100644 --- a/internal/scheduler/azure_hold_carry_integration_test.go +++ b/internal/scheduler/azure_hold_carry_integration_test.go @@ -36,14 +36,22 @@ func heldAzureAccounts(t *testing.T, store config.StoreInterface) []string { return nil } +func registerAzure(t *testing.T, store config.StoreInterface, enabled bool) string { + t.Helper() + acct := &config.CloudAccount{Name: "azure-" + uuid.NewString(), Provider: "azure", ExternalID: uuid.NewString(), Enabled: enabled} + require.NoError(t, store.CreateCloudAccount(context.Background(), acct)) + return acct.ID +} + // The hold marker lasts as long as the held rows: it survives every cycle that // does not re-price the account, and is dropped only by a cycle that does. func TestAzureHoldCarriedAcrossCycles(t *testing.T) { ctx := context.Background() - acctA, acctB := uuid.NewString(), uuid.NewString() + var acctA, acctB string newScheduler := func(t *testing.T) (*Scheduler, config.StoreInterface) { container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") store := config.NewPostgresStore(container.DB) + acctA, acctB = registerAzure(t, store, true), registerAzure(t, store, true) s := &Scheduler{config: store} s.persistCollection(ctx, nil, nil, nil, heldPartial(acctA)) require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) @@ -88,6 +96,38 @@ func TestAzureHoldCarriedAcrossCycles(t *testing.T) { require.Equal(t, []string{acctB}, heldAzureAccounts(t, store)) }) + t.Run("a deleted account's hold drops", func(t *testing.T) { + s, store := newScheduler(t) + require.NoError(t, store.DeleteCloudAccount(ctx, acctA)) + s.persistCollection(ctx, nil, azureOK(acctB), nil, nil) + require.Empty(t, heldAzureAccounts(t, store)) + }) + + t.Run("a disabled account's hold drops", func(t *testing.T) { + s, store := newScheduler(t) + acct, err := store.GetCloudAccount(ctx, acctA) + require.NoError(t, err) + acct.Enabled = false + require.NoError(t, store.UpdateCloudAccount(ctx, acct)) + s.persistCollection(ctx, nil, azureOK(acctB), nil, nil) + require.Empty(t, heldAzureAccounts(t, store)) + }) + + t.Run("an ambient hold clears once accounts are registered", func(t *testing.T) { + container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(container.DB) + s := &Scheduler{config: store} + s.persistCollection(ctx, nil, nil, nil, heldPartial()) + fresh, err := store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.NotNil(t, fresh.LastCollectionPartial) + registered := registerAzure(t, store, true) + s.persistCollection(ctx, nil, azureOK(registered), nil, nil) + fresh, err = store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.Nil(t, fresh.LastCollectionPartial) + }) + t.Run("an unresolved ambient hold survives an outright failure", func(t *testing.T) { container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") store := config.NewPostgresStore(container.DB) @@ -113,7 +153,7 @@ func TestPersistCollection_FailedMarkerWriteRollsBackUpsert(t *testing.T) { container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") store := config.NewPostgresStore(container.DB) s := &Scheduler{config: store} - acctA := uuid.NewString() + acctA := registerAzure(t, store, true) seedAdvisorRow(t, store) s.persistCollection(ctx, nil, nil, nil, heldPartial(acctA)) require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) From 19dab435a9dc7c428bfe2da95ef1aada49acdbe0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 06:41:36 +0100 Subject: [PATCH 17/27] fix(api): scope held_account_ids in the freshness payload to the caller GET /api/recommendations/freshness is AuthUser, so any view:recommendations caller received the platform account IDs of every held Azure account, which names accounts a scoped caller cannot see. Narrow the IDs to the caller's allowed accounts; unrestricted callers still see all. The refresh response does not carry the payload. Also fix the stale AuthAdmin comment. Co-Authored-By: Claude Opus 5.5 --- internal/api/freshness_held_scope_test.go | 59 +++++++++++++++++++++++ internal/api/handler_recommendations.go | 47 +++++++++++++++--- 2 files changed, 98 insertions(+), 8 deletions(-) create mode 100644 internal/api/freshness_held_scope_test.go diff --git a/internal/api/freshness_held_scope_test.go b/internal/api/freshness_held_scope_test.go new file mode 100644 index 000000000..2656ef424 --- /dev/null +++ b/internal/api/freshness_held_scope_test.go @@ -0,0 +1,59 @@ +package api + +import ( + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// GET /api/recommendations/freshness is AuthUser and the state row is global, +// so held_account_ids must only name accounts the caller may see. + +const ( + heldInScope = "11111111-1111-1111-1111-111111111111" + heldOutScope = "22222222-2222-2222-2222-222222222222" +) + +func heldFreshness(ids ...string) *config.RecommendationsFreshness { + return &config.RecommendationsFreshness{LastCollectionPartial: &config.RecommendationsCollectionPartial{ + Providers: []config.PartialProviderSweep{{ + Provider: "azure", IncompleteAccounts: len(ids), HeldAccountIDs: ids, + Reasons: map[string]int{config.PartialReasonPricingUnavailable: 1}, + }}, + }} +} + +func heldIDsFor(t *testing.T, accounts []string, stored *config.RecommendationsFreshness) []string { + t.Helper() + f := newRefreshFixture(t, viewRecommendationsOnly(), accounts) + f.store.ExpectedCalls = nil + f.store.On("GetRecommendationsFreshness", mock.Anything).Return(stored, nil) + res, err := NewRouter(f.handler).Route(t.Context(), "GET", "/api/recommendations/freshness", scopedRequest("")) + require.NoError(t, err) + got, ok := res.(*config.RecommendationsFreshness) + require.True(t, ok, "got %T", res) + require.NotNil(t, got.LastCollectionPartial) + return got.LastCollectionPartial.Providers[0].HeldAccountIDs +} + +func TestFreshness_HeldAccountsScopedToCaller(t *testing.T) { + stored := func() *config.RecommendationsFreshness { return heldFreshness(heldInScope, heldOutScope) } + + t.Run("scoped caller sees only its own held account", func(t *testing.T) { + assert.Equal(t, []string{heldInScope}, heldIDsFor(t, []string{heldInScope}, stored())) + }) + t.Run("unrestricted caller sees all", func(t *testing.T) { + assert.Equal(t, []string{heldInScope, heldOutScope}, heldIDsFor(t, nil, stored())) + }) + t.Run("caller with no matching accounts sees none", func(t *testing.T) { + assert.Empty(t, heldIDsFor(t, []string{"33333333-3333-3333-3333-333333333333"}, stored())) + }) + t.Run("the stored value is not mutated", func(t *testing.T) { + s := stored() + heldIDsFor(t, []string{heldInScope}, s) + assert.Equal(t, []string{heldInScope, heldOutScope}, s.LastCollectionPartial.Providers[0].HeldAccountIDs) + }) +} diff --git a/internal/api/handler_recommendations.go b/internal/api/handler_recommendations.go index 3a55527ef..8f8b5060d 100644 --- a/internal/api/handler_recommendations.go +++ b/internal/api/handler_recommendations.go @@ -4,6 +4,7 @@ package api import ( "context" "fmt" + "slices" "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -338,22 +339,52 @@ func stampComputeCapacity(rec *config.RecommendationRecord) { // null when unknown; issue #756), computed from the schedule rather than // stored. // -// Gated by view:recommendations permission inside the handler. The route -// itself inherits AuthAdmin (the router default) so the permission check -// is currently defense-in-depth, matching the pattern used by other -// scoped handlers in this package. +// The route is AuthUser: any caller holding view:recommendations reaches it, +// so the held account IDs in the payload are narrowed to the caller's allowed +// accounts (scopeHeldAccounts). func (h *Handler) getRecommendationsFreshness(ctx context.Context, req *events.LambdaFunctionURLRequest) (*config.RecommendationsFreshness, error) { - if _, err := h.requirePermission(ctx, req, "view", "recommendations"); err != nil { + session, err := h.requirePermission(ctx, req, "view", "recommendations") + if err != nil { return nil, err } freshness, err := h.config.GetRecommendationsFreshness(ctx) if err != nil { return nil, fmt.Errorf("failed to get recommendations freshness: %w", err) } - if freshness != nil { - freshness.NextCollectionAt = h.NextCollectionAt(h.clock()) + if freshness == nil { + return nil, nil + } + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + scoped := scopeHeldAccounts(freshness, scope) + scoped.NextCollectionAt = h.NextCollectionAt(h.clock()) + return scoped, nil +} + +// scopeHeldAccounts returns a copy of f whose held_account_ids name only +// accounts the scope allows. The state row is global, so naming an account the +// caller cannot see would leak that it exists. An unrestricted scope sees all +// of it; an account-less (ambient) hold has no IDs to show a restricted caller. +func scopeHeldAccounts(f *config.RecommendationsFreshness, scope auth.AccountScope) *config.RecommendationsFreshness { + out := *f + if scope.AllowsAll() || f.LastCollectionPartial == nil { + return &out + } + partial := *f.LastCollectionPartial + partial.Providers = slices.Clone(partial.Providers) + for i := range partial.Providers { + var allowed []string + for _, id := range partial.Providers[i].HeldAccountIDs { + if scope.Allows(id, "") { + allowed = append(allowed, id) + } + } + partial.Providers[i].HeldAccountIDs = allowed } - return freshness, nil + out.LastCollectionPartial = &partial + return &out } // getRecommendationDetail returns the per-id drill-down payload backing From 04920728ee9a471c26e8de732132b1a8e7089ab9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 06:41:45 +0100 Subject: [PATCH 18/27] test(purchase): cover the nil collection state and the pricing reason gate A missing collection state must refuse an Azure purchase, and a partial sweep without the pricing_unavailable reason must not. Co-Authored-By: Claude Opus 5.5 --- internal/purchase/azure_pricing_hold_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/purchase/azure_pricing_hold_test.go b/internal/purchase/azure_pricing_hold_test.go index 8eac27227..a9c4567b0 100644 --- a/internal/purchase/azure_pricing_hold_test.go +++ b/internal/purchase/azure_pricing_hold_test.go @@ -38,6 +38,12 @@ func TestAzurePricingHold(t *testing.T) { {"hold cleared by a good cycle", &acct, &config.RecommendationsFreshness{}, nil, false}, {"unheld account unaffected", &other, heldFreshness(acct), nil, false}, {"unreadable hold state refuses", &acct, nil, errors.New("database unavailable"), true}, + {"missing collection state refuses", &acct, nil, nil, true}, + // The reason gate: only a pricing_unavailable sweep holds rows. A sweep that + // is partial for another reason (here throttling) must not refuse a purchase. + {"partial sweep without the pricing reason does not refuse", &acct, &config.RecommendationsFreshness{LastCollectionPartial: &config.RecommendationsCollectionPartial{ + Providers: []config.PartialProviderSweep{{Provider: "azure", HeldAccountIDs: []string{acct}, Reasons: map[string]int{config.PartialReasonThrottled: 1}}}, + }}, nil, false}, {"unresolved ambient rec refused during an outage", nil, heldFreshness(), nil, true}, // Fail-closed by design: a rec without an account cannot be matched to the // hold list, so any Azure hold refuses it. From f80af07413d42e51437f0ca6256af53be7d9bb00 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 06:55:13 +0100 Subject: [PATCH 19/27] fix(config): clear the lint findings in the hold merge Avoid the err shadow in the upsert and drop the constant parameter of the test helper that registers Azure accounts. Co-Authored-By: Claude Opus 5.5 --- .../config/store_postgres_recommendations.go | 20 ++++++++++++------- .../azure_hold_carry_integration_test.go | 10 +++++----- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index 853947fd6..ce9508020 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -83,13 +83,9 @@ func (s *PostgresStore) UpsertRecommendationsWithPartial(ctx context.Context, co // The state row lock is taken last, after the row writes, so the lock order // is unchanged from the plain UPDATE this replaced. - partial, err = carryHoldInTx(ctx, tx, partial, successfulCollects) - if err != nil { - return err - } - partialJSON, err := encodeCollectionPartial(partial) - if err != nil { - return err + partialJSON, markerErr := lockedMarkerJSON(ctx, tx, partial, successfulCollects) + if markerErr != nil { + return markerErr } // last_collected_at only moves forward (issue #111). Two collections can @@ -128,6 +124,16 @@ func encodeCollectionPartial(partial *RecommendationsCollectionPartial) ([]byte, return encoded, nil } +// lockedMarkerJSON returns the encoded marker to write: partial with the +// previous Azure hold merged in under the state row lock. +func lockedMarkerJSON(ctx context.Context, tx pgx.Tx, partial *RecommendationsCollectionPartial, collects []SuccessfulCollect) ([]byte, error) { + merged, err := carryHoldInTx(ctx, tx, partial, collects) + if err != nil { + return nil, err + } + return encodeCollectionPartial(merged) +} + // carryHoldInTx locks the state row, then merges the previous Azure pricing // hold into partial. The lock serializes this read-merge-write against every // concurrent collection. An unreadable row or account list fails the tx. diff --git a/internal/scheduler/azure_hold_carry_integration_test.go b/internal/scheduler/azure_hold_carry_integration_test.go index 3d0f7adec..d1d33feef 100644 --- a/internal/scheduler/azure_hold_carry_integration_test.go +++ b/internal/scheduler/azure_hold_carry_integration_test.go @@ -36,9 +36,9 @@ func heldAzureAccounts(t *testing.T, store config.StoreInterface) []string { return nil } -func registerAzure(t *testing.T, store config.StoreInterface, enabled bool) string { +func registerAzure(t *testing.T, store config.StoreInterface) string { t.Helper() - acct := &config.CloudAccount{Name: "azure-" + uuid.NewString(), Provider: "azure", ExternalID: uuid.NewString(), Enabled: enabled} + acct := &config.CloudAccount{Name: "azure-" + uuid.NewString(), Provider: "azure", ExternalID: uuid.NewString(), Enabled: true} require.NoError(t, store.CreateCloudAccount(context.Background(), acct)) return acct.ID } @@ -51,7 +51,7 @@ func TestAzureHoldCarriedAcrossCycles(t *testing.T) { newScheduler := func(t *testing.T) (*Scheduler, config.StoreInterface) { container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") store := config.NewPostgresStore(container.DB) - acctA, acctB = registerAzure(t, store, true), registerAzure(t, store, true) + acctA, acctB = registerAzure(t, store), registerAzure(t, store) s := &Scheduler{config: store} s.persistCollection(ctx, nil, nil, nil, heldPartial(acctA)) require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) @@ -121,7 +121,7 @@ func TestAzureHoldCarriedAcrossCycles(t *testing.T) { fresh, err := store.GetRecommendationsFreshness(ctx) require.NoError(t, err) require.NotNil(t, fresh.LastCollectionPartial) - registered := registerAzure(t, store, true) + registered := registerAzure(t, store) s.persistCollection(ctx, nil, azureOK(registered), nil, nil) fresh, err = store.GetRecommendationsFreshness(ctx) require.NoError(t, err) @@ -153,7 +153,7 @@ func TestPersistCollection_FailedMarkerWriteRollsBackUpsert(t *testing.T) { container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") store := config.NewPostgresStore(container.DB) s := &Scheduler{config: store} - acctA := registerAzure(t, store, true) + acctA := registerAzure(t, store) seedAdvisorRow(t, store) s.persistCollection(ctx, nil, nil, nil, heldPartial(acctA)) require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) From 32a18fe5202f0fdd453af3f346b51608dad70413 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 07:18:48 +0100 Subject: [PATCH 20/27] fix(config): hold Azure prices while their stored rows exist The hold was dropped when its account was disabled or an account was registered, but disabling keeps the rows and no purchase path checks the Enabled flag, so the stale-priced rows stayed buyable. Keep a held account while stored Azure rows for it exist, and the ambient hold while rows with the nil account key exist; both end when the rows are deleted or pruned. The enabled-account read is replaced by a read of the stored rows' account keys in the same transaction. Adds tests for these rules and for a failed scope lookup in the freshness handler failing closed. Co-Authored-By: Claude Opus 5.5 --- internal/api/freshness_held_scope_test.go | 25 +++++++++++ internal/config/azure_hold_carry.go | 31 ++++++++------ internal/config/azure_hold_carry_test.go | 33 ++++++++------- .../azure_hold_concurrent_integration_test.go | 4 ++ .../config/store_postgres_recommendations.go | 14 +++---- .../azure_hold_carry_integration_test.go | 42 ++++++++++++++++--- 6 files changed, 108 insertions(+), 41 deletions(-) diff --git a/internal/api/freshness_held_scope_test.go b/internal/api/freshness_held_scope_test.go index 2656ef424..295826163 100644 --- a/internal/api/freshness_held_scope_test.go +++ b/internal/api/freshness_held_scope_test.go @@ -1,6 +1,7 @@ package api import ( + "errors" "testing" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" @@ -57,3 +58,27 @@ func TestFreshness_HeldAccountsScopedToCaller(t *testing.T) { assert.Equal(t, []string{heldInScope, heldOutScope}, s.LastCollectionPartial.Providers[0].HeldAccountIDs) }) } + +// A scope that cannot be established must fail closed: an error, never the +// unfiltered held IDs. +func TestFreshness_ScopeLookupFailureFailsClosed(t *testing.T) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, scopedToken).Return(&Session{UserID: scopedUserID}, nil).Maybe() + mockAuth.grantPermissionsScoped(viewRecommendationsOnly(), []string{heldInScope}) + kept := mockAuth.ExpectedCalls[:0] + for _, c := range mockAuth.ExpectedCalls { + if c.Method != "GetAllowedAccountsAPI" { + kept = append(kept, c) + } + } + mockAuth.ExpectedCalls = kept + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, errors.New("auth store unavailable")) + f := newRefreshFixtureWithAuth(t, mockAuth) + f.store.ExpectedCalls = nil + f.store.On("GetRecommendationsFreshness", mock.Anything).Return(heldFreshness(heldInScope, heldOutScope), nil) + + res, err := NewRouter(f.handler).Route(t.Context(), "GET", "/api/recommendations/freshness", scopedRequest("")) + + require.Error(t, err) + assert.Nil(t, res) +} diff --git a/internal/config/azure_hold_carry.go b/internal/config/azure_hold_carry.go index af0851c46..d3908c9ad 100644 --- a/internal/config/azure_hold_carry.go +++ b/internal/config/azure_hold_carry.go @@ -2,8 +2,13 @@ package config import ( "slices" + + "github.com/google/uuid" ) +// ambientAccountKey is the account_key of rows with no registered account. +var ambientAccountKey = uuid.Nil.String() + // carryAzureHold keeps the Azure pricing hold alive for as long as the held // rows are. Every upsert replaces last_collection_partial, but an account's // Advisor rows stay at their old price until a later cycle re-prices them, so @@ -12,17 +17,19 @@ import ( // ones are added, the rest carry over. That covers another outage, an outright // Azure failure and a run that skips Azure. // -// A held account that is no longer an enabled registered Azure account (enabled -// holds its IDs) is dropped, since it will never be collected again. A hold -// with no account IDs (an unresolved ambient identity) carries until an -// ambient Azure collection succeeds or registered Azure accounts exist, after -// which the ambient identity is no longer collected. -func carryAzureHold(prev, partial *RecommendationsCollectionPartial, collects []SuccessfulCollect, enabled map[string]bool) *RecommendationsCollectionPartial { +// A hold lasts only while stored Azure rows it protects exist: stored holds the +// account_key of every provider=azure row, read in the same transaction. A +// deleted account's rows cascade and pruned rows disappear, which ends the +// hold; a disabled account keeps its rows, so it stays held. A hold with no +// account IDs (an unresolved ambient identity) covers the ambient rows, keyed +// by the nil UUID, and carries until an ambient Azure collection succeeds or +// none remain. +func carryAzureHold(prev, partial *RecommendationsCollectionPartial, collects []SuccessfulCollect, stored map[string]bool) *RecommendationsCollectionPartial { held := previousAzureHold(prev) if held == nil { return partial } - carried, unresolved := stillHeld(held, collects, enabled) + carried, unresolved := stillHeld(held, collects, stored) if len(carried) == 0 && !unresolved { return partial } @@ -68,10 +75,10 @@ func previousAzureHold(partial *RecommendationsCollectionPartial) *PartialProvid return nil } -// stillHeld returns the previously held accounts this cycle did not re-price, -// that are still enabled, and whether an account-less (ambient) hold is still +// stillHeld returns the previously held accounts this cycle did not re-price +// that still have stored Azure rows, and whether an account-less (ambient) hold is still // unresolved. -func stillHeld(prev *PartialProviderSweep, collects []SuccessfulCollect, enabled map[string]bool) (carried []string, unresolved bool) { +func stillHeld(prev *PartialProviderSweep, collects []SuccessfulCollect, stored map[string]bool) (carried []string, unresolved bool) { repriced := map[string]bool{} ambientRepriced := false for _, sc := range collects { @@ -84,9 +91,9 @@ func stillHeld(prev *PartialProviderSweep, collects []SuccessfulCollect, enabled } } for _, id := range prev.HeldAccountIDs { - if !repriced[id] && enabled[id] { + if !repriced[id] && stored[id] { carried = append(carried, id) } } - return carried, len(prev.HeldAccountIDs) == 0 && !ambientRepriced && len(enabled) == 0 + return carried, len(prev.HeldAccountIDs) == 0 && !ambientRepriced && stored[ambientAccountKey] } diff --git a/internal/config/azure_hold_carry_test.go b/internal/config/azure_hold_carry_test.go index 6e06f9053..d2e3ba3da 100644 --- a/internal/config/azure_hold_carry_test.go +++ b/internal/config/azure_hold_carry_test.go @@ -19,31 +19,32 @@ func azureCollect(id *string) []SuccessfulCollect { func TestCarryAzureHold(t *testing.T) { a, b, c := "a", "b", "c" - enabled := map[string]bool{"a": true, "b": true, "c": true} + stored := map[string]bool{"a": true, "b": true, "c": true} + ambient := map[string]bool{ambientAccountKey: true} for name, tc := range map[string]struct { prev *RecommendationsCollectionPartial collects []SuccessfulCollect partial *RecommendationsCollectionPartial - enabled map[string]bool + stored map[string]bool want []string wantHold bool }{ - "no previous hold": {nil, nil, nil, enabled, nil, false}, - "nothing re-priced": {holdOf("a", "b"), nil, nil, enabled, []string{"a", "b"}, true}, - "one re-priced": {holdOf("a", "b"), azureCollect(&a), nil, enabled, []string{"b"}, true}, - "both re-priced": {holdOf("a", "b"), append(azureCollect(&a), azureCollect(&b)...), nil, enabled, nil, false}, - "other provider ignored": {holdOf("a"), []SuccessfulCollect{{Provider: "gcp", CloudAccountID: &a}}, nil, enabled, []string{"a"}, true}, - "new hold is merged": {holdOf("a", "b"), nil, holdOf("c"), enabled, []string{"a", "b", "c"}, true}, - "re-held account dedupes": {holdOf("a", "b"), nil, holdOf("a"), enabled, []string{"a", "b"}, true}, - "deleted account drops": {holdOf("a", "b"), nil, nil, map[string]bool{"b": true}, []string{"b"}, true}, - "disabled account drops": {holdOf("a"), nil, nil, map[string]bool{"c": true}, nil, false}, - "ambient carries": {holdOf(), nil, nil, nil, nil, true}, - "ambient carries a fail": {holdOf(), azureCollect(&c), nil, nil, nil, true}, - "ambient re-priced": {holdOf(), azureCollect(nil), nil, nil, nil, false}, - "ambient clears once registered": {holdOf(), nil, nil, enabled, nil, false}, + "no previous hold": {nil, nil, nil, stored, nil, false}, + "nothing re-priced": {holdOf("a", "b"), nil, nil, stored, []string{"a", "b"}, true}, + "one re-priced": {holdOf("a", "b"), azureCollect(&a), nil, stored, []string{"b"}, true}, + "both re-priced": {holdOf("a", "b"), append(azureCollect(&a), azureCollect(&b)...), nil, stored, nil, false}, + "other provider ignored": {holdOf("a"), []SuccessfulCollect{{Provider: "gcp", CloudAccountID: &a}}, nil, stored, []string{"a"}, true}, + "new hold is merged": {holdOf("a", "b"), nil, holdOf("c"), stored, []string{"a", "b", "c"}, true}, + "re-held account dedupes": {holdOf("a", "b"), nil, holdOf("a"), stored, []string{"a", "b"}, true}, + "account without rows drops": {holdOf("a", "b"), nil, nil, map[string]bool{"b": true}, []string{"b"}, true}, + "all rows gone drops": {holdOf("a"), nil, nil, map[string]bool{"c": true}, nil, false}, + "ambient carries while rows exist": {holdOf(), nil, nil, ambient, nil, true}, + "ambient survives account rows too": {holdOf(), azureCollect(&c), nil, map[string]bool{ambientAccountKey: true, "c": true}, nil, true}, + "ambient re-priced": {holdOf(), azureCollect(nil), nil, ambient, nil, false}, + "ambient drops with no ambient rows": {holdOf(), nil, nil, stored, nil, false}, } { t.Run(name, func(t *testing.T) { - got := carryAzureHold(tc.prev, tc.partial, tc.collects, tc.enabled) + got := carryAzureHold(tc.prev, tc.partial, tc.collects, tc.stored) if !tc.wantHold { assert.Nil(t, got) return diff --git a/internal/config/azure_hold_concurrent_integration_test.go b/internal/config/azure_hold_concurrent_integration_test.go index 30ecbc596..a668e069f 100644 --- a/internal/config/azure_hold_concurrent_integration_test.go +++ b/internal/config/azure_hold_concurrent_integration_test.go @@ -28,6 +28,10 @@ func TestUpsertRecommendationsWithPartial_ConcurrentCollectionKeepsHold(t *testi seedRecommendationCloudAccount(ctx, t, store, acctX, "azure", "sub-hold-x") seedRecommendationCloudAccount(ctx, t, store, acctY, "azure", "sub-hold-y") + // The hold protects X's stored rows, which must exist for it to carry. + require.NoError(t, store.UpsertRecommendations(ctx, time.Now().Add(-time.Hour), + []config.RecommendationRecord{azureAccountRec("row-x", acctX, "D2")}, nil)) + holder, err := db.Begin(ctx) require.NoError(t, err) t.Cleanup(func() { _ = holder.Rollback(ctx) }) diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index ce9508020..cfc297312 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -146,23 +146,23 @@ func carryHoldInTx(ctx context.Context, tx pgx.Tx, partial *RecommendationsColle if previousAzureHold(prev) == nil { return partial, nil } - rows, err := tx.Query(ctx, `SELECT id::text FROM cloud_accounts WHERE provider = 'azure' AND enabled`) + rows, err := tx.Query(ctx, `SELECT DISTINCT account_key::text FROM recommendations WHERE provider = 'azure'`) if err != nil { - return nil, fmt.Errorf("failed to list enabled azure accounts: %w", err) + return nil, fmt.Errorf("failed to list azure accounts with stored rows: %w", err) } defer rows.Close() - enabled := map[string]bool{} + stored := map[string]bool{} for rows.Next() { var id string if err := rows.Scan(&id); err != nil { - return nil, fmt.Errorf("failed to scan azure account id: %w", err) + return nil, fmt.Errorf("failed to scan azure account key: %w", err) } - enabled[id] = true + stored[id] = true } if err := rows.Err(); err != nil { - return nil, fmt.Errorf("failed to list enabled azure accounts: %w", err) + return nil, fmt.Errorf("failed to list azure accounts with stored rows: %w", err) } - return carryAzureHold(prev, partial, collects, enabled), nil + return carryAzureHold(prev, partial, collects, stored), nil } // successfulCollectArrays converts the SuccessfulCollect slice into the two diff --git a/internal/scheduler/azure_hold_carry_integration_test.go b/internal/scheduler/azure_hold_carry_integration_test.go index d1d33feef..bf34803ef 100644 --- a/internal/scheduler/azure_hold_carry_integration_test.go +++ b/internal/scheduler/azure_hold_carry_integration_test.go @@ -5,6 +5,7 @@ package scheduler import ( "context" "testing" + "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" @@ -43,6 +44,18 @@ func registerAzure(t *testing.T, store config.StoreInterface) string { return acct.ID } +// seedAzureAccountRows stores an old Azure row for each account, the rows a +// hold protects. +func seedAzureAccountRows(t *testing.T, store config.StoreInterface, accounts ...string) { + t.Helper() + recs := make([]config.RecommendationRecord, 0, len(accounts)) + for _, id := range accounts { + recs = append(recs, config.RecommendationRecord{ID: "row-" + id, CloudAccountID: &id, Provider: "azure", Service: "compute", + Region: "westeurope", ResourceType: "Standard_D2s_v3", Savings: 40, Count: 1, Term: 12, Payment: "upfront"}) + } + require.NoError(t, store.UpsertRecommendations(context.Background(), time.Now().Add(-time.Hour), recs, nil)) +} + // The hold marker lasts as long as the held rows: it survives every cycle that // does not re-price the account, and is dropped only by a cycle that does. func TestAzureHoldCarriedAcrossCycles(t *testing.T) { @@ -52,6 +65,7 @@ func TestAzureHoldCarriedAcrossCycles(t *testing.T) { container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") store := config.NewPostgresStore(container.DB) acctA, acctB = registerAzure(t, store), registerAzure(t, store) + seedAzureAccountRows(t, store, acctA, acctB) s := &Scheduler{config: store} s.persistCollection(ctx, nil, nil, nil, heldPartial(acctA)) require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) @@ -103,27 +117,42 @@ func TestAzureHoldCarriedAcrossCycles(t *testing.T) { require.Empty(t, heldAzureAccounts(t, store)) }) - t.Run("a disabled account's hold drops", func(t *testing.T) { + t.Run("a disabled account with rows stays held", func(t *testing.T) { s, store := newScheduler(t) acct, err := store.GetCloudAccount(ctx, acctA) require.NoError(t, err) acct.Enabled = false require.NoError(t, store.UpdateCloudAccount(ctx, acct)) s.persistCollection(ctx, nil, azureOK(acctB), nil, nil) - require.Empty(t, heldAzureAccounts(t, store)) + require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) }) - t.Run("an ambient hold clears once accounts are registered", func(t *testing.T) { + ambientHold := func(t *testing.T, withRows bool) (*Scheduler, config.StoreInterface) { container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") store := config.NewPostgresStore(container.DB) + if withRows { + seedAdvisorRow(t, store) + } s := &Scheduler{config: store} s.persistCollection(ctx, nil, nil, nil, heldPartial()) fresh, err := store.GetRecommendationsFreshness(ctx) require.NoError(t, err) require.NotNil(t, fresh.LastCollectionPartial) - registered := registerAzure(t, store) - s.persistCollection(ctx, nil, azureOK(registered), nil, nil) - fresh, err = store.GetRecommendationsFreshness(ctx) + return s, store + } + + t.Run("an ambient hold stays while ambient rows exist, even once accounts are registered", func(t *testing.T) { + s, store := ambientHold(t, true) + s.persistCollection(ctx, nil, azureOK(registerAzure(t, store)), nil, nil) + fresh, err := store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.NotNil(t, fresh.LastCollectionPartial) + }) + + t.Run("an ambient hold drops once no ambient rows remain", func(t *testing.T) { + s, store := ambientHold(t, false) + s.persistCollection(ctx, nil, azureOK(registerAzure(t, store)), nil, nil) + fresh, err := store.GetRecommendationsFreshness(ctx) require.NoError(t, err) require.Nil(t, fresh.LastCollectionPartial) }) @@ -132,6 +161,7 @@ func TestAzureHoldCarriedAcrossCycles(t *testing.T) { container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") store := config.NewPostgresStore(container.DB) s := &Scheduler{config: store} + seedAdvisorRow(t, store) collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips}) s.persistCollection(ctx, nil, nil, map[string]string{"azure": "boom"}, nil) fresh, err := store.GetRecommendationsFreshness(ctx) From 6efd10e53a4b4382e4ff3856f556b930917324e2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 07:52:32 +0100 Subject: [PATCH 21/27] test(config): pin the azure filter on the stored-rows read and sharpen the scope-failure test An ambient Azure hold must drop when only AWS rows keyed by the nil account remain. The freshness scope-failure test now also asserts no payload, no held IDs in the error and a server (non-client) error. Co-Authored-By: Claude Opus 5.5 --- internal/api/freshness_held_scope_test.go | 8 ++++++-- .../azure_hold_concurrent_integration_test.go | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/internal/api/freshness_held_scope_test.go b/internal/api/freshness_held_scope_test.go index 295826163..d9ef2c7e8 100644 --- a/internal/api/freshness_held_scope_test.go +++ b/internal/api/freshness_held_scope_test.go @@ -79,6 +79,10 @@ func TestFreshness_ScopeLookupFailureFailsClosed(t *testing.T) { res, err := NewRouter(f.handler).Route(t.Context(), "GET", "/api/recommendations/freshness", scopedRequest("")) - require.Error(t, err) - assert.Nil(t, res) + require.Error(t, err, "a failed scope lookup must not produce a payload: got %v", res) + assert.Nil(t, res, "nothing, filtered or not, may be returned") + assert.NotContains(t, err.Error(), heldOutScope) + assert.NotContains(t, err.Error(), heldInScope) + _, isClient := IsClientError(err) + assert.False(t, isClient, "a lookup failure is a server error, not a client one") } diff --git a/internal/config/azure_hold_concurrent_integration_test.go b/internal/config/azure_hold_concurrent_integration_test.go index a668e069f..b1fe838d4 100644 --- a/internal/config/azure_hold_concurrent_integration_test.go +++ b/internal/config/azure_hold_concurrent_integration_test.go @@ -61,3 +61,22 @@ func TestUpsertRecommendationsWithPartial_ConcurrentCollectionKeepsHold(t *testi require.NotNil(t, fresh.LastCollectionPartial, "the later commit dropped the other run's hold") require.Equal(t, []string{acctX}, fresh.LastCollectionPartial.Providers[0].HeldAccountIDs) } + +// The stored-rows read must be limited to Azure: an ambient Azure hold ends when +// no Azure ambient rows remain, even if another provider's ambient rows exist. +func TestUpsertRecommendationsWithPartial_AmbientHoldIgnoresOtherProviderRows(t *testing.T) { + ctx := context.Background() + store := config.NewPostgresStore(testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()).DB) + require.NoError(t, store.UpsertRecommendations(ctx, time.Now().Add(-time.Hour), + []config.RecommendationRecord{ambientRec("aws-ambient", "aws", "m5.large")}, nil)) + ambientHold := &config.RecommendationsCollectionPartial{Providers: []config.PartialProviderSweep{{ + Provider: "azure", IncompleteAccounts: 1, Reasons: map[string]int{config.PartialReasonPricingUnavailable: 1}, + }}} + require.NoError(t, store.UpsertRecommendationsWithPartial(ctx, time.Now().UTC(), nil, nil, ambientHold)) + + require.NoError(t, store.UpsertRecommendations(ctx, time.Now().UTC(), nil, nil)) + + fresh, err := store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.Nil(t, fresh.LastCollectionPartial, "AWS ambient rows must not keep an Azure hold alive") +} From c8e65e386aeac8025be04077eb2f8c00275f18a9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 09:17:05 +0100 Subject: [PATCH 22/27] fix(config): carry the ambient Azure hold across named account re-pricing An ambient hold merged with a named account's hold was recorded only as the named IDs, so once the named account re-priced, the marker cleared while stale ambient rows stayed purchasable at their old price. Record the ambient part as the nil UUID in the held IDs so it carries until an ambient collection succeeds or no ambient rows remain. Co-Authored-By: Claude Opus 5.5 --- internal/config/azure_hold_carry.go | 11 ++++++++- internal/config/azure_hold_carry_test.go | 31 ++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/internal/config/azure_hold_carry.go b/internal/config/azure_hold_carry.go index d3908c9ad..22dfdbfd1 100644 --- a/internal/config/azure_hold_carry.go +++ b/internal/config/azure_hold_carry.go @@ -23,7 +23,8 @@ var ambientAccountKey = uuid.Nil.String() // hold; a disabled account keeps its rows, so it stays held. A hold with no // account IDs (an unresolved ambient identity) covers the ambient rows, keyed // by the nil UUID, and carries until an ambient Azure collection succeeds or -// none remain. +// none remain. Once the hold also names accounts, the ambient part is recorded +// as the nil UUID in the held IDs, so it outlives the named accounts' re-pricing. func carryAzureHold(prev, partial *RecommendationsCollectionPartial, collects []SuccessfulCollect, stored map[string]bool) *RecommendationsCollectionPartial { held := previousAzureHold(prev) if held == nil { @@ -33,11 +34,16 @@ func carryAzureHold(prev, partial *RecommendationsCollectionPartial, collects [] if len(carried) == 0 && !unresolved { return partial } + if unresolved { + carried = append(carried, ambientAccountKey) + } return mergeCarriedHold(partial, carried) } // mergeCarriedHold adds the carried account IDs and the pricing_unavailable // reason to partial's Azure entry, creating the entry when this cycle had none. +// An entry that already records the pricing reason with no account IDs is this +// cycle's ambient outage; it gains the nil UUID so it survives a named hold. func mergeCarriedHold(partial *RecommendationsCollectionPartial, carried []string) *RecommendationsCollectionPartial { if partial == nil { partial = &RecommendationsCollectionPartial{} @@ -53,6 +59,8 @@ func mergeCarriedHold(partial *RecommendationsCollectionPartial, carried []strin } if sweep.Reasons[PartialReasonPricingUnavailable] == 0 { sweep.Reasons[PartialReasonPricingUnavailable] = 1 + } else if len(sweep.HeldAccountIDs) == 0 { + carried = append(carried, ambientAccountKey) } held := slices.Concat(sweep.HeldAccountIDs, carried) slices.Sort(held) @@ -86,6 +94,7 @@ func stillHeld(prev *PartialProviderSweep, collects []SuccessfulCollect, stored case sc.Provider != "azure": case sc.CloudAccountID == nil: ambientRepriced = true + repriced[ambientAccountKey] = true default: repriced[*sc.CloudAccountID] = true } diff --git a/internal/config/azure_hold_carry_test.go b/internal/config/azure_hold_carry_test.go index d2e3ba3da..dfa0949d4 100644 --- a/internal/config/azure_hold_carry_test.go +++ b/internal/config/azure_hold_carry_test.go @@ -38,8 +38,8 @@ func TestCarryAzureHold(t *testing.T) { "re-held account dedupes": {holdOf("a", "b"), nil, holdOf("a"), stored, []string{"a", "b"}, true}, "account without rows drops": {holdOf("a", "b"), nil, nil, map[string]bool{"b": true}, []string{"b"}, true}, "all rows gone drops": {holdOf("a"), nil, nil, map[string]bool{"c": true}, nil, false}, - "ambient carries while rows exist": {holdOf(), nil, nil, ambient, nil, true}, - "ambient survives account rows too": {holdOf(), azureCollect(&c), nil, map[string]bool{ambientAccountKey: true, "c": true}, nil, true}, + "ambient carries while rows exist": {holdOf(), nil, nil, ambient, []string{ambientAccountKey}, true}, + "ambient survives account rows too": {holdOf(), azureCollect(&c), nil, map[string]bool{ambientAccountKey: true, "c": true}, []string{ambientAccountKey}, true}, "ambient re-priced": {holdOf(), azureCollect(nil), nil, ambient, nil, false}, "ambient drops with no ambient rows": {holdOf(), nil, nil, stored, nil, false}, } { @@ -56,3 +56,30 @@ func TestCarryAzureHold(t *testing.T) { }) } } + +// An ambient hold merged with a named account's hold must stay an ambient hold +// after the named account re-prices: the stale ambient rows are still there. +func TestCarryAzureHold_AmbientSurvivesNamedReprice(t *testing.T) { + c := "c" + stored := map[string]bool{ambientAccountKey: true, "c": true} + + // Cycle 1: the ambient hold carries while this cycle holds account c. + merged := carryAzureHold(holdOf(), holdOf("c"), nil, stored) + require.NotNil(t, merged) + + // Cycle 2: c re-prices, the ambient identity does not. + got := carryAzureHold(merged, nil, azureCollect(&c), stored) + require.NotNil(t, got, "ambient rows still exist, so the hold must stay") + assert.Contains(t, got.Providers[0].HeldAccountIDs, ambientAccountKey) + + // Cycle 3: the ambient identity re-prices, which ends the hold. + assert.Nil(t, carryAzureHold(got, nil, azureCollect(nil), stored)) +} + +// An ambient pricing outage in this cycle plus a carried named hold keeps both. +func TestCarryAzureHold_ThisCycleAmbientKeptWithCarriedNamed(t *testing.T) { + stored := map[string]bool{ambientAccountKey: true, "a": true} + got := carryAzureHold(holdOf("a"), holdOf(), nil, stored) + require.NotNil(t, got) + assert.ElementsMatch(t, []string{"a", ambientAccountKey}, got.Providers[0].HeldAccountIDs) +} From 4238e1594815c850b48fdbae6a86193d2fbba6a0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 09:22:00 +0100 Subject: [PATCH 23/27] fix(config): count every carried Azure hold in incomplete_accounts mergeCarriedHold took the larger of the existing count and the number of held IDs, which undercounts when this cycle's incomplete accounts are not the held ones. Add each newly carried account to the count instead, without recounting this cycle's own ambient outage. Co-Authored-By: Claude Opus 5.5 --- internal/config/azure_hold_carry.go | 10 ++++++++- internal/config/azure_hold_carry_test.go | 26 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/internal/config/azure_hold_carry.go b/internal/config/azure_hold_carry.go index 22dfdbfd1..077a8c3ad 100644 --- a/internal/config/azure_hold_carry.go +++ b/internal/config/azure_hold_carry.go @@ -54,6 +54,7 @@ func mergeCarriedHold(partial *RecommendationsCollectionPartial, carried []strin idx = len(partial.Providers) - 1 } sweep := &partial.Providers[idx] + ownAmbient := false if sweep.Reasons == nil { sweep.Reasons = map[string]int{} } @@ -61,11 +62,18 @@ func mergeCarriedHold(partial *RecommendationsCollectionPartial, carried []strin sweep.Reasons[PartialReasonPricingUnavailable] = 1 } else if len(sweep.HeldAccountIDs) == 0 { carried = append(carried, ambientAccountKey) + ownAmbient = true } + before := len(sweep.HeldAccountIDs) held := slices.Concat(sweep.HeldAccountIDs, carried) slices.Sort(held) sweep.HeldAccountIDs = slices.Compact(held) - sweep.IncompleteAccounts = max(sweep.IncompleteAccounts, len(sweep.HeldAccountIDs)) + // Each newly held account adds to the incomplete count; this cycle's own + // ambient outage was already counted. + sweep.IncompleteAccounts += len(sweep.HeldAccountIDs) - before + if ownAmbient { + sweep.IncompleteAccounts-- + } return partial } diff --git a/internal/config/azure_hold_carry_test.go b/internal/config/azure_hold_carry_test.go index dfa0949d4..8ad2f1cb5 100644 --- a/internal/config/azure_hold_carry_test.go +++ b/internal/config/azure_hold_carry_test.go @@ -83,3 +83,29 @@ func TestCarryAzureHold_ThisCycleAmbientKeptWithCarriedNamed(t *testing.T) { require.NotNil(t, got) assert.ElementsMatch(t, []string{"a", ambientAccountKey}, got.Providers[0].HeldAccountIDs) } + +// Carried accounts add to the incomplete count instead of being absorbed into a +// larger count that came from other failures this cycle. +func TestCarryAzureHold_IncompleteAccountsCountsEachCarriedAccount(t *testing.T) { + stored := map[string]bool{"a": true, "b": true, "x": true, ambientAccountKey: true} + for name, tc := range map[string]struct { + prev, partial *RecommendationsCollectionPartial + want int + }{ + "carried on top of other failures": {holdOf("a", "b"), withIncomplete(holdOf("x"), 3), 5}, + "own ambient outage counted once": {holdOf("a"), withIncomplete(holdOf(), 1), 2}, + "nothing this cycle": {holdOf("a", "b"), nil, 2}, + "ambient carried": {holdOf(), nil, 1}, + } { + t.Run(name, func(t *testing.T) { + got := carryAzureHold(tc.prev, tc.partial, nil, stored) + require.NotNil(t, got) + assert.Equal(t, tc.want, got.Providers[0].IncompleteAccounts) + }) + } +} + +func withIncomplete(p *RecommendationsCollectionPartial, n int) *RecommendationsCollectionPartial { + p.Providers[0].IncompleteAccounts = n + return p +} From be4e320d666b3b631317397038fb919b81bad84e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 09:23:08 +0100 Subject: [PATCH 24/27] fix(scheduler): hold Azure accounts on pricing-outage skips in a mixed sweep gapFromPartialSubscriptions recorded only the skipped count, so a sweep that lost a subscription and also skipped Advisor rows for a price outage kept those rows without a hold. Classify the skips with the same helper tolerateSkippedRecommendations uses, so the gap is marked pricingUnavailable and the caller names the account as held. Co-Authored-By: Claude Opus 5.5 --- internal/scheduler/partial_sweep.go | 33 +++++++++++++----------- internal/scheduler/partial_sweep_test.go | 22 ++++++++++++++++ 2 files changed, 40 insertions(+), 15 deletions(-) diff --git a/internal/scheduler/partial_sweep.go b/internal/scheduler/partial_sweep.go index f50d86f71..e11c8d66a 100644 --- a/internal/scheduler/partial_sweep.go +++ b/internal/scheduler/partial_sweep.go @@ -75,21 +75,23 @@ func tolerateSkippedRecommendations(providerName string, err error) (*sweepGap, return nil, err } logging.Warnf("%s recommendations partial, keeping the rest: %v", providerName, err) - // A skip that means "the price could not be fetched" says nothing about - // the row being invalid, so its previous-cycle row must survive. - outage := countPricingOutageSkips(p.SkipReasons()) - g := &sweepGap{ - skippedRecommendations: p.SkippedCount(), - skippedOnly: outage == 0, - pricingUnavailable: outage > 0, - reasons: map[string]int{}, - } - if outage > 0 { - g.reasons[config.PartialReasonPricingUnavailable] = outage - } + g := &sweepGap{reasons: map[string]int{}} + g.recordSkips(p.SkippedCount(), p.SkipReasons()) + g.skippedOnly = !g.pricingUnavailable return g, nil } +// recordSkips adds dropped recommendations to the gap. A skip that means "the +// price could not be fetched" says nothing about the row being invalid, so its +// previous-cycle row must survive: it marks the gap pricingUnavailable. +func (g *sweepGap) recordSkips(count int, reasons []string) { + g.skippedRecommendations += count + if outage := countPricingOutageSkips(reasons); outage > 0 { + g.pricingUnavailable = true + g.reasons[config.PartialReasonPricingUnavailable] += outage + } +} + // withHeld records accountID as held when the gap is a pricing outage. An // empty accountID (an unresolved ambient identity) cannot be named. func (g *sweepGap) withHeld(accountID string) *sweepGap { @@ -165,10 +167,11 @@ func gapFromPartialSubscriptions(p *azureprovider.PartialSubscriptionFailureErro failedScopes: max(p.Attempted-p.Succeeded, len(p.Failed)), reasons: map[string]int{}, } - // A mixed sweep also lost invalid recommendations; the count is for the - // user warning only and does not affect eviction (skippedOnly stays false). + // A mixed sweep also lost invalid recommendations. The count is for the + // user warning only and does not affect eviction (skippedOnly stays false), + // but a pricing outage among them holds the account like a skips-only one. if sk := p.Skips(); sk != nil { - g.skippedRecommendations = sk.SkippedCount() + g.recordSkips(sk.SkippedCount(), sk.SkipReasons()) } for _, f := range p.Failed { g.reasons[classifySweepCause(f.Err)]++ diff --git a/internal/scheduler/partial_sweep_test.go b/internal/scheduler/partial_sweep_test.go index fd8938ab6..91e64ee6f 100644 --- a/internal/scheduler/partial_sweep_test.go +++ b/internal/scheduler/partial_sweep_test.go @@ -274,3 +274,25 @@ func TestAppendPartialProvider(t *testing.T) { assert.Equal(t, "azure", got.Providers[0].Provider) assert.NotNil(t, got.Providers[0].Reasons, "reasons must encode as {} rather than null") } + +// A mixed Azure sweep (a lost subscription plus Advisor pricing-outage skips on +// a reachable one) must hold the account like a skips-only outage does, or its +// rows stay purchasable at the previous cycle's price. +func TestGapFromPartialSubscriptions_PricingOutageSkipsHoldAccount(t *testing.T) { + outage := []string{ + `advisor: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, + `advisor: price lookup unavailable: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, + } + failed := []azureprovider.SubscriptionFailure{{SubscriptionID: "sub-b", Err: errors.New("403 authorization failed")}} + + gap, err := tolerateIncompleteSweep("azure", azureprovider.NewPartialSubscriptionFailure(2, failed, outage)) + require.NoError(t, err) + + held := gap.forAccount().withHeld("acct-1") + assert.True(t, held.pricingUnavailable) + assert.Equal(t, []string{"acct-1"}, held.heldAccountIDs) + assert.True(t, held.holdsEviction(), "the lost subscription still holds eviction") + sweep := held.toProviderSweep("azure") + assert.Positive(t, sweep.Reasons[config.PartialReasonPricingUnavailable]) + assert.Equal(t, []string{"acct-1"}, sweep.HeldAccountIDs) +} From c9a2891db6812ce667c06d7549599fd1cc06e41b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 09:23:25 +0100 Subject: [PATCH 25/27] docs(api): describe the carried pricing hold in RecommendationsCollectionPartial Co-Authored-By: Claude Opus 5.5 --- internal/api/openapi.yaml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index fd99a0507..a50893265 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -6985,8 +6985,11 @@ components: nullable: true description: > Non-null when the most recent persisted collection kept rows from - at least one incomplete sweep. Cleared by the next collection whose - sweeps are all complete. Carries counts and reason categories only. + at least one incomplete sweep, or while a pricing hold carries + forward from an earlier collection. Cleared by the next collection + whose sweeps are all complete and that leaves no hold. Carries counts + and reason categories, plus held_account_ids filtered to the + caller's account scope. required: [providers] properties: providers: From e68a36b77512adc42b3a39d0571cbec79265b7ec Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 09:31:50 +0100 Subject: [PATCH 26/27] fix(scheduler): strip the subscription prefix from skips in a mixed Azure sweep The multi-subscription client the scheduler gets when scope is ambiguous prefixes each aggregated skip with ": ", so Advisor pricing outage skips in a mixed sweep never matched the outage prefixes and the account was not held. Strip that leading segment in gapFromPartialSubscriptions only. Co-Authored-By: Claude Opus 5.5 --- internal/scheduler/partial_sweep.go | 18 +++++++++++++++++- internal/scheduler/partial_sweep_test.go | 9 ++++++++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/internal/scheduler/partial_sweep.go b/internal/scheduler/partial_sweep.go index e11c8d66a..1e1419f0e 100644 --- a/internal/scheduler/partial_sweep.go +++ b/internal/scheduler/partial_sweep.go @@ -81,6 +81,22 @@ func tolerateSkippedRecommendations(providerName string, err error) (*sweepGap, return g, nil } +// trimSubscriptionPrefixes drops the ": " that the Azure +// multi-subscription client puts before each skip it aggregates +// (recommendations_multi_subscription.go:399 at 61111428), so the reasons match +// the same prefixes as a single-subscription sweep. Only a leading segment +// followed by an Advisor skip is removed; the single-sweep path never calls it. +func trimSubscriptionPrefixes(reasons []string) []string { + out := make([]string, len(reasons)) + for i, reason := range reasons { + out[i] = reason + if sub, rest, ok := strings.Cut(reason, ": "); ok && sub != "" && !strings.ContainsAny(sub, " :") && strings.HasPrefix(rest, "advisor: ") { + out[i] = rest + } + } + return out +} + // recordSkips adds dropped recommendations to the gap. A skip that means "the // price could not be fetched" says nothing about the row being invalid, so its // previous-cycle row must survive: it marks the gap pricingUnavailable. @@ -171,7 +187,7 @@ func gapFromPartialSubscriptions(p *azureprovider.PartialSubscriptionFailureErro // user warning only and does not affect eviction (skippedOnly stays false), // but a pricing outage among them holds the account like a skips-only one. if sk := p.Skips(); sk != nil { - g.recordSkips(sk.SkippedCount(), sk.SkipReasons()) + g.recordSkips(sk.SkippedCount(), trimSubscriptionPrefixes(sk.SkipReasons())) } for _, f := range p.Failed { g.reasons[classifySweepCause(f.Err)]++ diff --git a/internal/scheduler/partial_sweep_test.go b/internal/scheduler/partial_sweep_test.go index 91e64ee6f..9f002fb0c 100644 --- a/internal/scheduler/partial_sweep_test.go +++ b/internal/scheduler/partial_sweep_test.go @@ -285,7 +285,14 @@ func TestGapFromPartialSubscriptions_PricingOutageSkipsHoldAccount(t *testing.T) } failed := []azureprovider.SubscriptionFailure{{SubscriptionID: "sub-b", Err: errors.New("403 authorization failed")}} - gap, err := tolerateIncompleteSweep("azure", azureprovider.NewPartialSubscriptionFailure(2, failed, outage)) + // The multi-subscription client Provider.GetRecommendationsClient returns + // when scope is ambiguous builds each skip as ": " + // (recommendations_multi_subscription.go:399 at 61111428). + prefixed := make([]string, len(outage)) + for i, r := range outage { + prefixed[i] = "0f4c1a52-8d3e-4b7a-9c61-2e5d7a9b3c10: " + r + } + gap, err := tolerateIncompleteSweep("azure", azureprovider.NewPartialSubscriptionFailure(2, failed, prefixed)) require.NoError(t, err) held := gap.forAccount().withHeld("acct-1") From bf874d9c361180b01e210eccd442a17a89dbb321 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 10:05:34 +0100 Subject: [PATCH 27/27] fix(scheduler): strip the subscription prefix on the skips-only Azure path too A skips-only multi-subscription sweep returns a plain PartialResultError whose skips carry the same ": " prefix, so its pricing outage was never held. Trim the prefix inside recordSkips, shared by both paths. Document that held_account_ids can hold the nil UUID for the ambient identity, shown only to unrestricted callers. Co-Authored-By: Claude Opus 5.5 --- internal/api/openapi.yaml | 3 +++ internal/config/types.go | 4 +++- internal/scheduler/partial_sweep.go | 6 +++--- internal/scheduler/partial_sweep_test.go | 17 +++++++++++++++++ 4 files changed, 26 insertions(+), 4 deletions(-) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index a50893265..cb971c99e 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -7042,6 +7042,9 @@ components: previous-cycle Azure Advisor rows are held because the price catalog was unreachable (reason pricing_unavailable). Their reservations cannot be purchased until a collection re-prices them. + The nil UUID (00000000-0000-0000-0000-000000000000) stands for the + ambient identity, which has no registered account. It is returned + only to callers with an unrestricted account scope. items: type: string format: uuid diff --git a/internal/config/types.go b/internal/config/types.go index e2577c84b..7401e528f 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -980,7 +980,9 @@ type PartialProviderSweep struct { Reasons map[string]int `json:"reasons"` // HeldAccountIDs lists the platform cloud-account UUIDs (not cloud // account numbers) whose previous-cycle rows are held through a pricing - // outage; the purchase path refuses their recommendations. + // outage; the purchase path refuses their recommendations. It can contain + // the nil UUID (all zeros) for the ambient identity, which has no account; + // the API shows it only to callers with an unrestricted account scope. HeldAccountIDs []string `json:"held_account_ids,omitempty"` } diff --git a/internal/scheduler/partial_sweep.go b/internal/scheduler/partial_sweep.go index 1e1419f0e..a510d0bbc 100644 --- a/internal/scheduler/partial_sweep.go +++ b/internal/scheduler/partial_sweep.go @@ -85,7 +85,7 @@ func tolerateSkippedRecommendations(providerName string, err error) (*sweepGap, // multi-subscription client puts before each skip it aggregates // (recommendations_multi_subscription.go:399 at 61111428), so the reasons match // the same prefixes as a single-subscription sweep. Only a leading segment -// followed by an Advisor skip is removed; the single-sweep path never calls it. +// followed by an Advisor skip is removed, so unprefixed reasons pass through. func trimSubscriptionPrefixes(reasons []string) []string { out := make([]string, len(reasons)) for i, reason := range reasons { @@ -102,7 +102,7 @@ func trimSubscriptionPrefixes(reasons []string) []string { // previous-cycle row must survive: it marks the gap pricingUnavailable. func (g *sweepGap) recordSkips(count int, reasons []string) { g.skippedRecommendations += count - if outage := countPricingOutageSkips(reasons); outage > 0 { + if outage := countPricingOutageSkips(trimSubscriptionPrefixes(reasons)); outage > 0 { g.pricingUnavailable = true g.reasons[config.PartialReasonPricingUnavailable] += outage } @@ -187,7 +187,7 @@ func gapFromPartialSubscriptions(p *azureprovider.PartialSubscriptionFailureErro // user warning only and does not affect eviction (skippedOnly stays false), // but a pricing outage among them holds the account like a skips-only one. if sk := p.Skips(); sk != nil { - g.recordSkips(sk.SkippedCount(), trimSubscriptionPrefixes(sk.SkipReasons())) + g.recordSkips(sk.SkippedCount(), sk.SkipReasons()) } for _, f := range p.Failed { g.reasons[classifySweepCause(f.Err)]++ diff --git a/internal/scheduler/partial_sweep_test.go b/internal/scheduler/partial_sweep_test.go index 9f002fb0c..62d48be62 100644 --- a/internal/scheduler/partial_sweep_test.go +++ b/internal/scheduler/partial_sweep_test.go @@ -303,3 +303,20 @@ func TestGapFromPartialSubscriptions_PricingOutageSkipsHoldAccount(t *testing.T) assert.Positive(t, sweep.Reasons[config.PartialReasonPricingUnavailable]) assert.Equal(t, []string{"acct-1"}, sweep.HeldAccountIDs) } + +// A skips-only multi-subscription sweep returns a plain PartialResultError +// (recommendations_multi_subscription.go:421 at 61111428) whose skips carry the +// same ": " prefix; its outage must hold the account too. +func TestTolerateIncompleteSweep_PrefixedPricingOutageSkipsHoldAccount(t *testing.T) { + skips := []string{ + `0f4c1a52-8d3e-4b7a-9c61-2e5d7a9b3c10: advisor: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, + `0f4c1a52-8d3e-4b7a-9c61-2e5d7a9b3c10: advisor: price lookup unavailable: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, + } + gap, err := tolerateIncompleteSweep("azure", &azureprovider.PartialResultError{Skipped: skips}) + require.NoError(t, err) + + held := gap.forAccount().withHeld("acct-1") + assert.True(t, held.pricingUnavailable) + assert.True(t, held.holdsEviction(), "the held rows must survive eviction") + assert.Equal(t, []string{"acct-1"}, held.heldAccountIDs) +}