fix(config): upsert recommendations on the 11-column index with platform and tenancy

[cristim]

Summary

PR-C of #806. The recommendations upsert now uses ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy), i.e. the 11-column recommendations_natural_key_az_tp_idx added by migration 000111 (#815). The insert column list is unchanged; the generated platform and tenancy columns are never written and are not in DO UPDATE SET. ReplaceRecommendations has no ON CONFLICT. This is the only ON CONFLICT / natural-key SQL on the table in internal/, scripts/, cmd/, tests/.

Tests (real PostgreSQL 17, local throwaway cluster):

• New TestPostgresStore_UpsertRecommendations_PlatformTenancyArbiter: same-key re-upsert updates in place and the generated columns carry the payload values; regional non-EC2 row (platform='', tenancy='') updates in place; a row written by pre-PR-C code (platform/tenancy only in details) is the conflict target, not duplicated.
• TestPostgresStore_UpsertRecommendations_AZArbiter legacy-row case: its seeded payload had only availability_zone in details, which now maps to platform '' and no longer matches the rec it upserts (23505 on the 9-col index). The seed now includes platform/tenancy, as the old collector always stored on zonal EC2 recs.
• internal/purchase/azure_pricing_integration_test.go:106: MigrateToVersion 109 -> 111 (R7). Re-grep of every MigrateToVersion user outside migrations/: that file (105, 106, 111), internal/api/nullable_purchase_cost_integration_test.go (100, 101; never calls the recommendations store), and the testhelpers skip-guard tests (version 42; no store). Only the Azure test needed the bump.

Known transitional defect (accepted window, until 000119)

With both indexes live (schema 111, before 000119), the 11-col arbiter does not cover the 9-col index. Reproduced on PG17 at schema 111: one collection run upserts EC2 m5.large as Linux, a later run upserts the same 9-tuple as Windows. The second insert hits recommendations_natural_key_az_idx (23505), the transaction aborts (eviction runs after the insert in the same tx), so UpsertRecommendations fails on every later run and the recommendation cache is not refreshed until migration 000119 drops the 9-col index. Main does not fail this probe. The same window was accepted and documented for #810 in the AZ chain.

The window is short: merge PR-E (000119) to dev right after PR-C is deployed to dev; in prod, PR-C and PR-E release together (apply only the prerequisite migrations through 000111 using the true pending count, release PR-C, then apply the drop-index migration). The multi-platform/tenancy batch test belongs to PR-E (R1). No test pins the transitional failure here; PR-E removes the cause.

Release runbook (PR-C and PR-E release together)

1. Read prod's current schema version first. Do not assume 110.
2. Dispatch database-migration.yml with steps set to the true pending count so that only 000111 applies. The workflow's default (steps empty/0) is all and would also apply 000119 once PR-E is on main.
3. Release PR-C code to prod.
4. Only then dispatch 000119 (PR-E). Never apply 000119 before PR-C is live: old code's 9-col ON CONFLICT has no arbiter once that index is dropped and every collection fails.
5. Merge PR-C only after DEV has clean schema >=111 and the valid unique 11-column arbiter. Authorized read-only AWS DEV verification on 2026-10-14 found schema 118, dirty=false, and the exact valid unique 11-column index. PR-E is published at 1f79ebe with migration 000119, after PR-C's successful AWS DEV deployment. Its original 000112 head remains preserved by a backup reference. PR-E requires green exact-head CI before merge.

Rollback

Once 000119 is applied, rolling back PR-C code needs down steps=1 on 000119 first; otherwise the old code runs without a 9-col arbiter.

Refs #806

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 54 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 60 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 1a247d93-160e-4ef7-80c5-7e8cf01472b4

---

📥 Commits

Reviewing files that changed from the base of the PR and between 8230ff9 and 3ca3721.

---

📒 Files selected for processing (3)

• internal/config/store_postgres_recommendations.go
• internal/config/store_postgres_recommendations_test.go
• internal/purchase/azure_pricing_integration_test.go

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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

[cristim]

Gate verdict (Opus 5.5, claude-opus-5-5): CHANGES REQUESTED at 3ca3721

Not merged. The code change itself is correct; one real, reproduced regression in the both-indexes window is undocumented and the PR body claims the switch is not observable.

Verified OK

• ON CONFLICT list (store_postgres_recommendations.go:231) matches the 000111 index recommendations_natural_key_az_tp_idx column for column and in order. Insert list unchanged; DO UPDATE SET has no generated column. No other ON CONFLICT / natural-key SQL on recommendations in internal/, scripts/, cmd/, tests/.
• azure_pricing_integration_test.go:107 bump 109 -> 111 is required (PR-C SQL needs the 000111 columns/index); other MigrateToVersion users (internal/api/nullable_purchase_cost_integration_test.go 100/101, testhelpers skip guard 42) never call the recommendations store.
• Labels mirror fix(recommendations): EC2 tenancy and platform missing from the recommendations natural key and rec ID #806, Refs #806 present, git-secrets clean on changed files, branch up to date with main (8230ff9).
• Local, throwaway PostgreSQL 17 (testhelpers patched in a git archive copy to use it), go test -tags integration -run 'TestPostgresStore_UpsertRecommendations' ./internal/config/, go1.26.9: all PASS at head, including the new PlatformTenancyArbiter and the edited AZArbiter subtests.

Finding 1 (blocking until documented and the rollout decided): cross-run platform/tenancy flip fails every collection while both indexes are live, and stays failing

Probe (schema 111, i.e. dev after this merges and prod between runbook steps 3 and 4): run 1 upserts an EC2 rec m5.large us-east-1 12mo no-upfront with platform Linux/UNIX; run 2 upserts the same 9-tuple with platform Windows (single rec, not a multi-platform batch).

• main (8230ff9, 9-col arbiter): run 2 err = <nil> (updates in place). PASS.
• this PR (3ca3721): run 2 failed to insert recommendations batch: ... recommendations_natural_key_az_idx (SQLSTATE 23505). FAIL.

The 11-col arbiter finds no conflict, the insert hits the 9-col index, and the whole UpsertRecommendations tx aborts. Because eviction runs after the insert in the same tx, the stale row is never evicted, so every later collection for that account fails the same way until 000112 drops the 9-col index. The same applies to a tenancy flip. This is exactly why the AZArbiter legacy seed had to gain platform/tenancy: that edit is legitimate for real collector output, but the old seed was itself an instance of this hazard.

This is different from the documented limit (multi-platform batch in ONE run, which already fails on main with 21000): single-platform-per-9-tuple accounts that collect fine today can start failing after this merge. The PR body sentence "by itself, the arbiter switch is not observable by a test until 000112" is incorrect.

Requested:

1. Document this case in the PR body (and the PR-E body): what triggers it, that it is sticky until 000112, and that it affects dev from this merge until PR-E's 000112 is applied.
2. Owner/author decision on the window: either keep it minimal (PR-E merged to dev right after PR-C; prod 000112 dispatched in the same maintenance window as the PR-C release, not "later"), or mitigate in code. Plan R2 currently keeps PR-E unmerged until PR-C is in prod, which leaves dev exposed for that whole period.

Not verified

• Dev schema version: deploys for 8230ff9 (PR-B, contains 000111) succeeded on AWS/Azure/GCP, but auto-migrate is non-fatal and the schema version was not read from dev (no secret access here). Still needs to be confirmed before merge.
• CI at this SHA: Unit/Integration Tests, Build Docker Image, Security Scanning still pending at review time.

Mutation testing: not run (store SQL arbiter change, not a money/auth path; optional per policy).

[cristim]

Corrected the PR body: the 'not observable' claim was wrong. The transitional 23505 (Linux then Windows on one 9-tuple while both indexes are live) is now stated with its effect, the accepted window, and the dev/prod sequencing. No code change; head SHA unchanged (3ca3721). No test added for the transitional failure; PR-E removes the cause.

[cristim]

Gate re-check (Opus 5.5, claude-opus-5-5): CONDITIONAL APPROVAL at 3ca3721, not merged

Finding 1 closed, per the coordinator ruling. The transitional window is accepted, as it was for #810. The body now:

• documents the cross-run Linux -> Windows 23505 case on the 9-col index and its effect: the tx aborts, UpsertRecommendations fails on every later run, and the recommendation cache is not refreshed until 000112;
• drops the "not observable" claim;
• keeps the runbook (read prod's schema version first; dispatch database-migration.yml with steps limited so only 000111 applies, because the default all also applies 000112 once PR-E is on main; release PR-C; then 000112; never 000112 before PR-C is live) and the rollback note (down steps=1 on 000112 before rolling back PR-C code).

Minor wording inconsistency, non-blocking: the transitional section says "steps limited to 1", while runbook step 2 says "the true pending count". Step 2 is the correct rule. Step 4's "Only then" should be read as "immediately after", which matches the transitional section.

State at this SHA: all checks green, including Unit Tests, Integration Tests, E2E, Security Scanning and CI Success. mergeStateStatus CLEAN. Labels match #806 exactly (priority/p1, severity/high, urgency/this-sprint, impact/many, effort/m, type/bug, triaged). Local PG17 evidence is in the previous gate comment.

Dev verification (public read-only /version and /health, 2026-10-12T20:42Z):

• Azure dev: git_sha 8230ff9 (PR-B, contains 000111); migrations: healthy, last run 4m6s ago. Verified.
• GCP dev: git_sha 20f6f4f (contains 000111); migrations: healthy, last run 1m9s ago. Verified.
• AWS Lambda dev: git_sha 20f6f4f, status healthy. The Lambda /health payload has no migrations check, and auto-migrate is non-fatal, so its schema version could not be verified without a CloudWatch or DB read, which this gate does not do.
• Deploy runs succeeded for ca5597b (38298524002/77/3984) and 8230ff9 (38303526938/834/858).

Conditions to merge:

1. Someone with access confirms AWS dev schema_migrations.version = 111 and dirty = false (or a CloudWatch log line showing the Lambda migration to 111 succeeded).
2. The head is BEHIND main by fix(frontend): include platform and tenancy in EC2 recommendation cell key #818 (frontend only; mergeStateStatus still CLEAN). The gate rule is not to merge a BEHIND head. Run gh pr update-branch 819 (merge method), confirm the PR's own diff is unchanged, wait for fresh green CI on the new head, then merge with --match-head-commit.

[cristim]

Independent Sol-medium review, two adversarial passes, exact head 3ca3721: no blocking findings within the previously accepted PR-E sequencing.

Source checks: the 11-column conflict target in internal/config/store_postgres_recommendations.go:231 exactly matches migration 000111:33. Insert and update omit generated columns. Scheduler conversion and persistence preserve platform, tenancy and AZ.

Fresh macOS evidence: build passed; focused race-enabled store integration cases passed on native PostgreSQL 17. An independent Recommendation -> real converter -> persistCollection -> PostgreSQL upsert -> store-read scenario preserved Windows/dedicated/AZ fields, updated savings from 10 to 25, and retained one row. After removing only the scratch 9-column index, exact-head code passed; substituting the parent store failed with SQLSTATE 42P10; restoring exact-head code passed again. Production source was byte-compared against the head afterwards. The Azure provider regression uses mocks. Scratch server stopped. This is synthetic-fixture database evidence, not live provider collection or DEV writes.

Separate actual AWS DEV read-only evidence: BEGIN READ ONLY, bounded timeouts, SELECT-only queries and ROLLBACK through an existing SSM route. Schema 118, dirty=false. Both natural-key indexes are valid and unique; the 11-column index matches the exact target. Armed multi-step Savings Plans aggregate count is zero. Owned session cleanup completed; no credential files or data writes.

Current PR state rechecked: OPEN, CLEAN, head unchanged; CI Success and Run pre-commit hooks completed successfully.

The retained 9-column index can still cause SQLSTATE 23505 for cross-platform or cross-tenancy changes until PR-E lands. The existing accepted sequence requires PR-E immediately after PR-C deploys to DEV and joint production release. Its migration must be renumbered beyond main's occupied versions; local 000119 preparation is reviewed but unpublished.

The affected scenario is the SQL persistence path. Actual DEV schema/index inspection and the native PostgreSQL production-store roundtrip with provider-model-shaped records cover it. Synthetic inputs and mocked provider execution are labelled above; this SQL fix does not require a real purchase or cloud write.

Two additional source-check passes confirmed the representative record against pinned common model revision 9dae64b216d4: pkg/common/types.go:201,232 defines Recommendation and typed details; :555-560 defines ComputeDetails JSON platform/tenancy/scope/availability_zone. providers/aws/recommendations/parser_services.go:159-172 preserves the platform and derives tenancy/scope/AZ; :97-105 accepts dedicated tenancy. Windows/dedicated/us-east-1a therefore match the production model. All eleven identity dimensions match the SQL target and migration index; account normalization, AZ derivation, platform and tenancy use the existing migration expressions. The reviewer approves normal merge at the named head under the accepted PR-E deployment sequence.