Skip to content

fix(azure): invalidate stale recommendation prices before purchase - #655

Merged
cristim merged 5 commits into
mainfrom
fix/azure-rec-units-612
Oct 8, 2026
Merged

cristim merged 5 commits into
mainfrom
fix/azure-rec-units-612

Conversation

@cristim

@cristim cristim commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Azure recommendation costs collected before the upstream units fix can understate purchase spend caps. This change invalidates stored Azure pricing and refuses a purchase when approved costs differ from the current recommendation. Azure purchases also require migration 106 to be cleanly applied: a failed migration cannot leave positive same-ID legacy prices eligible for purchase.

Refusals persist a failed execution and return HTTP 409 through the approval/direct API paths. Storage errors and audit-save failures remain server errors.

Rollout

Validation at 6464a133e3bddd9c6d520e775fda966dbcae60e7

  • Rebased onto merged fix(azure): roll out reservation fixes and preserve exchange scopes #475 at 1a62ea7 without conflicts. Both fresh exact-head workflows passed: CI - Build & Test (37754030297) and pre-commit (37754030390). Normal squash merge f2d60df matches the independently reviewed combined tree.
  • Fresh independent full-PR source review verified the resolved corrected Azure provider, scheduler mapping and PostgreSQL price persistence, found no actionable findings and replayed the real Approve path using native PostgreSQL, real migration/pricing persistence, and a mocked purchase provider.
  • The identical fixture on the previous production source failed with one provider call where zero was expected. Fixed source refuses migration 105, dirty 106, missing migration metadata, and invalidated prices; successful migration plus real recommendation upsert permits exactly one mock purchase.
  • Native PostgreSQL fixtures also exercised migration 106, SQL reapplication, rollback, and same-key replacement of old-unit prices. No cloud purchase was made.
  • Final affected packages passed go test -race -short -count=1 -p=1 ./internal/config ./internal/purchase ./internal/api; pinned golangci-lint reported zero issues. Normal commit hooks passed.
  • CodeRabbit's green status at this SHA says review skipped because the PR is draft. It is not review evidence. The project permits exact-revision local verification plus thorough independent review as the review gate; the owner's override permits the available independent reviewer and realistic local fixtures.

Sibling audit
The CLI CSV importer ignores upfront/monthly price columns and uses AWS-only purchase clients; the proposed stale Azure CSV replay defect was not reproduced. The MCP known-zero upfront preview follow-up is tracked in LeanerCloud/cloud-commitments-mcp#44.

Refs #612

@cristim cristim added triaged Item has been triaged priority/p0 Drop everything; same-day fix severity/critical Major harm when it happens urgency/now Drop other things impact/many Affects most users effort/m Days type/bug Defect labels Oct 7, 2026
@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 4e089d64-31d0-4fe8-9838-27dae6252471
📥 Commits

Reviewing files that changed from the base of the PR and between 1b15d74 and 6464a13.

📒 Files selected for processing (11)
  • internal/api/handler_purchases_retry_redrive_test.go
  • internal/api/purchase_pricing_test.go
  • internal/config/errors.go
  • internal/config/store_postgres_azure_pricing_test.go
  • internal/config/store_postgres_recommendations.go
  • internal/config/types.go
  • internal/purchase/armed_redrive_test.go
  • internal/purchase/azure_pricing.go
  • internal/purchase/azure_pricing_integration_test.go
  • internal/purchase/azure_pricing_test.go
  • internal/purchase/manager_test.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.


📝 Walkthrough

Walkthrough

Migration 106 clears stored Azure cost fields. The recommendation store can check migration readiness. Before purchase execution, the manager compares Azure recommendation prices with stored values and refuses stale prices. The API maps stale-pricing errors to HTTP 409.

Changes

Azure pricing validation

Layer / File(s) Summary
Invalidate and replace stored Azure prices
internal/database/postgres/migrations/000106_invalidate_azure_recs_lookback_units.*, internal/database/postgres/migrations/000106_invalidate_azure_recs_lookback_units_test.go, internal/config/store_postgres_recommendations_test.go
Migration 106 sets Azure upfront and monthly cost fields to JSON null. Tests cover migration behavior and replacement of stored recommendation values during upsert.
Validate Azure prices before purchase
internal/config/errors.go, internal/config/types.go, internal/config/store_postgres_recommendations.go, internal/config/store_postgres_azure_pricing_test.go, internal/purchase/azure_pricing.go, internal/purchase/azure_pricing_test.go, internal/purchase/azure_pricing_integration_test.go, internal/purchase/manager.go, internal/purchase/*_test.go, internal/api/handler_purchases_retry_redrive_test.go
The recommendation store checks migration readiness when requested. The manager checks stored Azure prices before purchase execution. Tests cover stale and matching prices, migration states, rounding, entry points, and Azure retry and redrive cases.
Expose stale pricing and verify purchase pricing
internal/api/handler_purchases.go, internal/api/purchase_pricing_test.go
The API maps stale Azure pricing to HTTP 409 and preserves audit-loss errors in token actions. Tests cover commitment calculations, stored-price caps, and error mappings.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant executeAndFinalize
  participant staleAzurePricingRefusal
  participant ListStoredRecommendations
  participant PurchaseExecution
  executeAndFinalize->>staleAzurePricingRefusal: check Azure recommendation prices
  staleAzurePricingRefusal->>ListStoredRecommendations: load stored Azure recommendations
  ListStoredRecommendations-->>staleAzurePricingRefusal: return stored recommendations
  staleAzurePricingRefusal-->>executeAndFinalize: return validation result
  executeAndFinalize->>PurchaseExecution: execute when validation succeeds
Loading

Merge Risk: ⚪ Minimal · up to 6464a

The change invalidates stale Azure prices and refuses purchases when approved costs differ from the current recommendation, returning HTTP 409. No actionable merge-blocking risk is evident from the supplied context.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 15 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preventing purchases that use stale Azure recommendation prices.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@cristim

cristim commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor
✅ 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.

@coderabbitai

coderabbitai Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Add Carrot credits or activate Agent usage billing to use Autopilot

@cristim

cristim commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

Current head: 1b15d74.

The lint follow-up replaced large range-value copies with indexed reads and removed an unused test initializer. Pinned golangci-lint v2.10.1, Go 1.26.6 build, purchase/API race-short tests and normal commit hooks passed. All CI jobs and the pre-commit workflow passed on this head.

CodeRabbit's completed walkthrough covers all 12 changed files from 3131779 to this head and reports no actionable comments; the inline review-comment list is empty. Its advisory docstring-coverage warning is declined: boilerplate comments on private pricing helpers and regression tests would not clarify their behavior. No review threads were resolved or suppressed. The later Autopilot billing prompt is not a review finding; no paid action was attempted.

The published follow-up is LeanerCloud/cloud-commitments-mcp#44. The rollout/history correction is recorded on #612.

This remains a draft. Local verification is mock/fixture based. The required real Azure scenario and independent exact claude-opus-5-5 reviewed-SHA verdict remain pending. No merge, deployment or purchase has been performed.

@cristim

cristim commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

Independent final-head review: 1b15d74, base 3131779.

A fresh Codex reviewer inspected all 12 changed files through the live GitHub diff and local source. No actionable findings across completeness, correctness, security, bugs, duplication, over-engineering, reuse and scope. The reviewer traced approval/direct/scheduled/redrive execution, scaled pricing, nil versus zero, failed-status persistence, audit-error precedence, spend caps, migration and recollection.

Verification is local fixture/mock evidence, permitted by the owner's latest merge-gate override. Earlier fail-first checks exercise the manager's actual provider-dispatch path; unchanged migration SQL has successful up/reapply/down evidence. Fresh build passed. Fresh full suite passed: Go 1.26.6, GOMAXPROCS=2, go test -race -short -count=1 -p=1 ./..., exit 0. Three unrelated timing failures in the initial parallel run passed unchanged in a focused controlled-load run and this full rerun. The reviewer's own runtime attempts stopped before assertions with transient Go cache/standard-library lookup errors, so those attempts are not counted as behavioral proof. Fresh migration up106 and recommendation upsert passed; the rollback repeat hit a Docker read-only-filesystem error. Earlier rollback proof uses identical migration files.

No live Azure account acceptance, purchase or deployment is claimed. Deployment still requires the fixed collector rollout through #475 and quiescing old collectors before migration. CodeRabbit completed normally on this head with zero actionable comments, so no CR quota waiver is used.

The removed exact-model and mandatory live-account gates are not merge blockers. Main-merge sequencing is with the coordinator: active push-to-main deployment workflows conflict with the retained no-deployment instruction. Merge must use an authorized path respecting that constraint, followed by a fresh unchanged-head, green-CI and mergeability check. No deployment workflow or repository setting has been changed by this worker.

@cristim
cristim marked this pull request as ready for review October 8, 2026 06:20
Refuse Azure execution while migration 106 is missing or dirty, so a
failed or skipped migration cannot leave matching old prices purchasable.
Preserve database failures and failed-execution audit handling.

Real PostgreSQL approval fixtures fail on the previous head with one
mocked provider call and pass after the readiness gate and recollection.
@cristim
cristim force-pushed the fix/azure-rec-units-612 branch from 4f8b96b to 6464a13 Compare October 8, 2026 09:03
@cristim

cristim commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

Independent adversarial review: no actionable findings at 6464a133e3bddd9c6d520e775fda966dbcae60e7, after integrating #475. The reviewer checked the resolved Azure provider's authoritative consumption pricing, scheduler mapping, PostgreSQL persistence and every shared Azure purchase guard entry.

The reviewer independently replayed TestApproveAzurePricingRequiresMigrationAndRecollection with Go 1.26.6, -race -tags=integration -p=1 -count=1: PASS, 5.509s. Pricing and migrations use real native PostgreSQL; execution persistence and cloud purchases are mocked. Parent-source replay actually dispatched one mocked purchase where zero was expected. The affected config/purchase/API race suites also passed at this head.

Main subsequently advanced to 13ba642fa272345403241eed9fd35f1567202c55 with only the owner-authorized CLAUDE.md policy amendment. Independent combined-tree verification confirms tree 8216e929869af619485447a843c148accc0c7e06 differs from the reviewed head only in that documentation file. Runtime source and exact-head proof are unchanged.

CodeRabbit is optional under the project review gate and the owner's current instruction; skipped or throttled bot checks are not counted as independent review evidence. Merge remains conditional on fresh green exact-head CI, unchanged SHA, clean mergeability and normal branch protections. Existing collectors must be quiesced during rollout before migration 106; no live cloud purchase was performed.

@cristim
cristim merged commit f2d60df into main Oct 8, 2026
25 checks passed
@cristim

cristim commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

Post-merge verification at f2d60df86209eea85d81e9091c20832bd670a65f: CI - Build & Test (37755443515), pre-commit (37755443370), and frontend-build-sentinel (37755443517) all completed successfully.

The AWS Lambda (37755443364), Azure Container Apps (37755443511), and GCP Cloud Run (37755443305) development deployment runs remain pending. Their preparation jobs passed; build/deploy jobs have no assigned runner. No pending environment approval request was returned. Read-only workflow watchers remain active. Deployment success and deployed purchase behavior are not claimed.

The actual merged tree equals the independently reviewed combined tree; the real PostgreSQL approval regression and affected race suites passed before merge. No cloud purchase was made. Existing #612 retains the historical execution limitation, and MCP #44 retains its separate preview follow-up. No new follow-up issue was needed for this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/many Affects most users priority/p0 Drop everything; same-day fix severity/critical Major harm when it happens triaged Item has been triaged type/bug Defect urgency/now Drop other things

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant