fix(scheduler): propagate ambient fallback GetRecommendations error

[cristim]

Problem

COR-05 from the 2026-06-10 codebase review: in internal/scheduler/scheduler.go, the AWS ambient/self-account path (fetchAndConvert) retries GetRecommendations with the global default term/payment when the primary sweep returns zero recommendations, but discards the retry's error entirely (recs, _ =). A misconfigured DefaultPayment/DefaultTerm or a Cost Explorer failure on the fallback is permanently invisible; the operator just sees zero recommendations with no signal. The deprecated converters feeding the params also carried stale doc comments claiming they "silently default" to NoUpfront/OneYear/SevenDays, when they actually return the empty enum (which Cost Explorer rejects with a validation error).

Fix

• fetchAndConvert now handles the fallback error and fails loud: the error is wrapped with provider, term, payment, and lookback context and returned, so the failure surfaces through the normal collection error path (failed-provider accounting, freshness banner, logs) instead of silently presenting an empty result.
• Fixed the three stale Deprecated: doc comments in providers/aws/recommendations/converters.go to describe the actual empty-enum behavior (doc-only; the converter behavior itself is unchanged and out of scope here).

Provider-factory wiring is intentionally untouched (tracked separately as ARCH-12 LeanerCloud/cloud-commitments-platform#41).

Test evidence

• Regression test TestScheduler_CollectAWSRecommendations_FallbackError replicates the real scenario: primary GetAllRecommendations returns zero recs, the fallback GetRecommendations returns a CE validation error. It asserts the error is propagated (errors.Is on the cause), carries the fallback/term/payment context, and that no recommendations are returned.
• Verified the test FAILS on pre-fix code (stashed the scheduler.go change: "An error is expected but got nil") and passes with the fix.
• go build ./... clean; go test ./internal/scheduler/... ./providers/aws/recommendations/... : 70 passed.

Closes #1168

Summary by CodeRabbit

• Bug Fixes

  • When the initial recommendation lookup returns no results, the fallback lookup now stops and returns an error if the fallback request fails.
  • Returned errors now include the provider plus the computed term, payment, and lookback values for clearer troubleshooting.

• Tests

  • Added a regression test ensuring fallback lookup errors are propagated and no recommendations are returned on failure.

• Documentation

  • Updated legacy converter/deprecation comments to match current behavior for unrecognized values (empty result instead of silent defaults).

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 17580d5e-1d2f-4f2a-b46f-02de0298f29b

📥 Commits

Reviewing files that changed from the base of the PR and between db2b24a and aeabbc8.

📒 Files selected for processing (4)

• internal/scheduler/scheduler.go
• internal/scheduler/scheduler_test.go
• providers/aws/recommendations/converters.go
• providers/aws/recommendations/converters_test.go

✅ Files skipped from review due to trivial changes (2)

• providers/aws/recommendations/converters.go
• providers/aws/recommendations/converters_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• internal/scheduler/scheduler_test.go
• internal/scheduler/scheduler.go

---

📝 Walkthrough

Walkthrough

The scheduler fallback path now returns an error when the fallback recommendation fetch fails after an empty primary result. AWS recommendation converter comments and tests now describe unsupported values as empty strings rather than default enum values.

Changes

Scheduler fallback error propagation

Layer / File(s)	Summary
Fallback error handling and regression test internal/scheduler/scheduler.go, internal/scheduler/scheduler_test.go	The fallback GetRecommendations call now returns a wrapped error when the initial recommendation sweep is empty, and a regression test covers the propagated failure and nil result.

AWS converter comment updates

Layer / File(s)	Summary
Legacy wrapper and test comment wording providers/aws/recommendations/converters.go, providers/aws/recommendations/converters_test.go	Comments for the legacy payment, term, and lookback converters, plus their regression tests, now describe unsupported inputs as producing empty enum strings rather than default enum values.

Estimated code review effort: 2 (Simple) | ~10 minutes

Related issue

• #1168: The scheduler fallback path previously swallowed GetRecommendations failures, and this PR changes that behavior and updates the related converter wording.

Possibly related PRs

• LeanerCloud/CUDly#1343: Also changes internal/scheduler/scheduler.go fallback recommendation error handling in the same area of code.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main fix: propagating the scheduler's fallback GetRecommendations error.
Linked Issues check	✅ Passed	The PR addresses #1168 by surfacing the fallback error on zero-result sweeps and correcting the stale converter docs.
Out of Scope Changes check	✅ Passed	The doc comment updates are in scope because the linked issue explicitly calls for fixing the stale converter documentation.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/cor-05-fix

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Adversarial review (PR #1230)

Reviewed against the COR-05 finding (#1168) and the risk surfaces in the brief: silent fallback path, error-type preservation, errgroup ctx propagation, caller behaviour with the new error, "no recommendations" vs "fetch failed" distinguishability, and sibling silent-fallback patterns. Fix is correct, narrowly scoped, well tested. One in-scope docstring sync pushed (db2b24a5a); no follow-ups filed.

Verified

• The bug is closed: internal/scheduler/scheduler.go:901 now captures err (was recs, _ =) and wraps it with provider / term / payment / lookback context via fmt.Errorf("... %w", err). The fallback GetRecommendations failure can no longer present as a silent "zero recommendations" outcome. ✓
• Caller behaviour confirmed end-to-end: the propagated error lands in collectAllProviders at scheduler.go:367 (failedProviders[providerName] = out.err.Error()), then in persistCollection at :400 it's persisted via SetRecommendationsCollectionError -> recommendations_state.last_collection_error, which drives the freshness banner. The operator sees the actual failure (term/payment/lookback included), not "0 recs". ✓
• feedback_no_silent_fallbacks.md satisfied: no caller falls back to "0 recommendations" on this error. The ambient path (collectAWSAmbient at :610) returns the error to collectAWSRecommendations (:467-470) which returns it to collectAllProviders; per-account path (collectAWSForAccount at :724,:735) routes through fanOutPerAccount which counts it as one account failure. Both terminate the fallback's "0 recs" pretence. ✓
• feedback_ctx_cancel_terminal.md satisfied: the %w wrapping preserves the error chain, so errors.Is(err, context.Canceled) / errors.Is(err, context.DeadlineExceeded) still works for callers that want to short-circuit on cancellation. The fan-out at collectAllProviders:352-355 already does the post-g.Wait() ctx.Err() check per feedback_errgroup_ctx_err.md. ✓
• feedback_empty_string_vs_error.md satisfied: caller can now distinguish "no recommendations available" (recs=[], err=nil) from "fallback fetch failed" (recs=nil, err!=nil). The new test asserts both invariants (require.ErrorIs + assert.Nil(t, recs)). ✓
• Sibling silent-fallback sweep clean: grep -n ", _ = .*Get" internal/scheduler/scheduler.go returns nothing in production code; the four ambient/per-account helper functions (collectAWSAmbient, collectAzureAmbient, collectGCPAmbient, collectAzureForAccount) all already propagate GetRecommendationsClient/GetAllRecommendations errors via wrapped fmt.Errorf. No remaining log-and-continue paths to fix. ✓
• Regression test correctness (scheduler_test.go:1604-1644): replicates the real failing scenario (primary GetAllRecommendations returns [] -> fallback GetRecommendations returns CE ValidationException), asserts require.ErrorIs(err, fallbackErr) (so the underlying cause is reachable), Contains on term=3yr / payment=all-upfront / default term/payment fallback (so the wrap context is asserted), and Nil(recs) (so the empty-slice anti-pattern can't sneak back in). Mocks register t.Cleanup(...AssertExpectations(t)) per feedback_mock_assert_expectations.md. Confirmed FAILS pre-fix and PASSES post-fix per the PR description; locally go test -race ./internal/scheduler/... passes 102/102 and go test github.com/LeanerCloud/CUDly/providers/aws/recommendations/... passes 331/331. ✓
• Doc-only fix in converters.go: the three deprecated wrappers (convertPaymentOption / convertTermInYears / convertLookbackPeriod) actually return the empty ("") enum for unknown values (verified by reading the bodies + their *E siblings) -- the previous docstring claim of "silently defaults to NoUpfront/OneYear/SevenDays" was wrong, and the new docstrings accurately describe the empty-enum-then-CE-rejects shape. ✓
• Out-of-scope acknowledged: the legacy RI path at providers/aws/recommendations/client.go:119-121 still sends those empty enums to CE, yielding a confusing "invalid PaymentOption: ''" rather than a meaningful "unsupported payment option XYZ". This is tracked by fix(aws/recommendations): protect RateLimiter against concurrent access (closes #271) #865/fix(aws): payment-option/engine/term correctness gaps in AWS RI service clients #1075 (referenced in both the original and updated docstrings). ✓

Fix pushed

db2b24a5a - docs(aws/recommendations): align converters_test docstrings with empty-enum behaviour. The PR rightly fixed the stale "silently defaults to NoUpfront/OneYear/SevenDays" docstrings in converters.go, but the sibling converters_test.go had the SAME stale claim in 9 places (test docstrings + per-case comments). Mirrored the fix into the test file to keep the source-of-truth comment consistent across the pair. No assertions modified; tests still pass. Mirrors the in-scope precedent the PR set, no scope creep.

Follow-up filed

None. The RI-path empty-enum issue is already tracked by #865/#1075. The pre-existing TestScheduler_CollectAWSRecommendations_FallbackToFiltered (scheduler_test.go:1565-1602) lacks AssertExpectations cleanup -- a minor pre-existing gap, not introduced or worsened by this PR; not worth a dedicated issue when the broader test-hygiene sweep is more cost-effective.

UNSTABLE state

Four failing checks (Lint Code, Integration Tests, Security Scanning, plus the CI Success aggregator) are pre-existing on main -- the latest CI - Build & Test run on main (27833571269 on 451a70f7) fails the identical four jobs. Not caused by this 3-file diff. Not blocking.

Verdict

LGTM. CR re-ping below.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Rebased onto origin/main (31 commits ahead).

Conflict resolved in internal/scheduler/scheduler.go: main had introduced a partial fix that logged a Warnf on the fallback error but continued silently with empty recommendations. This PR's intent is to fail loud (return the error), which is the correct behavior per the project's no-silent-fallbacks policy. Resolution keeps the PR's return nil, fmt.Errorf(...) with full context (provider, term, payment, lookback) over main's logging.Warnf approach.

Gates passed:

• go build ./... clean
• go vet ./internal/scheduler/... ./providers/aws/recommendations/... clean
• go test ./providers/aws/recommendations/... 70 passed
• go test ./internal/scheduler/... -run TestScheduler_CollectAWSRecommendations 9 passed (including TestScheduler_CollectAWSRecommendations_FallbackError)

New head: aeabbc8

[cristim]

@coderabbitai review

[coderabbitai]

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 4 minutes and 1 seconds before sending another message.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Merged on the substantive CI signal (all Lint/Unit/Integration/Security/Build/E2E/Validate-Terraform green; only the non-blocking tflint pre-commit flake red) closes #1168.