Problem
PR LeanerCloud/cloud-commitments-cli#1215 ported the AWS 08-H4 all-attempted-failed guard to Azure (providers/azure/recommendations.go mergeServiceResults) and GCP (providers/gcp/recommendations.go mergeRegionResults). Both surface the total failure as
fmt.Errorf("all %d <provider> recommendation services failed: %w", failures, lastErr)This is correct -- the underlying error is wrapped so errors.Is(err, context.Canceled) and similar checks still work. But callers cannot positively distinguish "every attempted call errored on this provider" from "one network blip on a single service" without parsing the format string. AWS has the identical shape (providers/aws/recommendations/client.go line 415).
The scheduler currently treats both shapes the same (any error from the rec client -> account in FailedCount), so the missing sentinel is not a behavioural bug today. It becomes one the moment we want to do anything provider-aware with the failure: emit a distinct metric for "credential expired across the whole provider" vs "one service flaked", surface the all-failed shape in the dashboard's freshness banner, or rate-limit a retry differently for the two cases.
Proposed fix
Introduce a single typed sentinel (e.g. var ErrAllRecServicesFailed = errors.New("all recommendation services failed")) in pkg/common (or a small providers/recerrors package next to providers/), and wrap as
return nil, fmt.Errorf("all %d Azure recommendation services failed: %w", failures, errors.Join(ErrAllRecServicesFailed, lastErr))(or a custom struct wrapping both) so the three sites can be reached with errors.Is(err, ErrAllRecServicesFailed). Update the AWS, Azure, and GCP guards in lockstep. Add a unit test on each site that the sentinel is reachable.
Out of scope (do not pile on)
Memory
Per feedback_no_silent_fallbacks.md and feedback_prefer_typed_enums.md, the sentinel makes the contract explicit and avoids relying on string-matching at any future caller that wants to react to this specific shape.
Findings from the 2026-09-02 codebase audit
Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.
A08-020 (medium)
Related to the sentinel work: GCP has a branch that bypasses the guard entirely rather than reporting through it. GetRecommendations (providers/gcp/recommendations.go:125-128) returns ([]common.Recommendation{}, nil) when getRegions fails and isPermissionError is true. That is precisely the outcome mergeRegionResults in the same file exists to prevent, whose comment at :178-189 says returning (recs, nil) on a total failure makes a broken run indistinguishable from 'no savings available' and lets the scheduler count the account as succeeded, evict its previously collected rows and clear last_collection_error (COR-03). The classifier is also loose: isPermissionError falls back to a substring test for both '403' and 'permission' on any error string (:411-412), so an unrelated failure can take the silent-empty branch. A typed permission error the scheduler can log at WARN without treating the sweep as a successful zero-result collection would fit alongside ErrAllRecServicesFailed. Audit finding A08-020.
Problem
PR LeanerCloud/cloud-commitments-cli#1215 ported the AWS 08-H4 all-attempted-failed guard to Azure (
providers/azure/recommendations.gomergeServiceResults) and GCP (providers/gcp/recommendations.gomergeRegionResults). Both surface the total failure asThis is correct -- the underlying error is wrapped so
errors.Is(err, context.Canceled)and similar checks still work. But callers cannot positively distinguish "every attempted call errored on this provider" from "one network blip on a single service" without parsing the format string. AWS has the identical shape (providers/aws/recommendations/client.goline 415).The scheduler currently treats both shapes the same (any error from the rec client -> account in
FailedCount), so the missing sentinel is not a behavioural bug today. It becomes one the moment we want to do anything provider-aware with the failure: emit a distinct metric for "credential expired across the whole provider" vs "one service flaked", surface the all-failed shape in the dashboard's freshness banner, or rate-limit a retry differently for the two cases.Proposed fix
Introduce a single typed sentinel (e.g.
var ErrAllRecServicesFailed = errors.New("all recommendation services failed")) inpkg/common(or a smallproviders/recerrorspackage next toproviders/), and wrap as(or a custom struct wrapping both) so the three sites can be reached with
errors.Is(err, ErrAllRecServicesFailed). Update the AWS, Azure, and GCP guards in lockstep. Add a unit test on each site that the sentinel is reachable.Out of scope (do not pile on)
Memory
Per
feedback_no_silent_fallbacks.mdandfeedback_prefer_typed_enums.md, the sentinel makes the contract explicit and avoids relying on string-matching at any future caller that wants to react to this specific shape.Findings from the 2026-09-02 codebase audit
Added by an automated audit of
3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd(tip oforigin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report:docs/audits/codebase-audit-2026-09-02.md.A08-020 (medium)
Related to the sentinel work: GCP has a branch that bypasses the guard entirely rather than reporting through it. GetRecommendations (providers/gcp/recommendations.go:125-128) returns ([]common.Recommendation{}, nil) when getRegions fails and isPermissionError is true. That is precisely the outcome mergeRegionResults in the same file exists to prevent, whose comment at :178-189 says returning (recs, nil) on a total failure makes a broken run indistinguishable from 'no savings available' and lets the scheduler count the account as succeeded, evict its previously collected rows and clear last_collection_error (COR-03). The classifier is also loose: isPermissionError falls back to a substring test for both '403' and 'permission' on any error string (:411-412), so an unrelated failure can take the silent-empty branch. A typed permission error the scheduler can log at WARN without treating the sweep as a successful zero-result collection would fit alongside ErrAllRecServicesFailed. Audit finding A08-020.