[cristim]

Part of #1232. This is a follow-up to #1411; the two shapes come from its independent review. Item 1 of #1232 is handled separately, so this PR does not close the issue.

Problem: a retried legacy EC2 execution can write a purchase suppression whose variant doesn't match the variant that was actually bought, so a later recommendation for that variant can be bought again. Two legacy row shapes cause this:

• Tenancy spelling: Details written before 2026-05-20 carry tenancy shared, and current recs carry default. The two never compared equal.
• AZ only in Details: rows saved from 2026-10-09 to 2026-10-12 hold the AZ only in details.availability_zone. The variant read only the AvailabilityZone field, so it got AZ "" and missed the zonal rec.

Change:

• RecommendationRecord.SuppressionVariant() builds the variant for both the suppression builder and the scheduler's matcher. For AWS EC2 only (ec2 or compute), it now maps tenancy shared to default, and it falls back to the Details AZ when the field is empty. When both are set, the field wins.
• PurchaseSuppression.AppliesTo also normalises tenancy on both sides of the comparison, so suppression rows already stored with shared still match.

Tests:

• TestBuildSuppressions_LegacyShapesCanonicalised has 6 cases: shared tenancy, the compute slug, a Details-only AZ, the field winning over Details, a modern rec unchanged, and a non-EC2 rec untouched.
• TestPurchaseSuppression_AppliesTo_LegacySharedTenancy.
• TestApplySuppressions_LegacyEC2Shapes has 4 scheduler cases.
• Mutation probes:
  • Removing the AZ fallback fails the config and scheduler tests.
  • Removing the AppliesTo normalisation fails both as well.
  • Removing the tenancy normalisation in SuppressionVariant fails the config test.

Local:

• go build ./... passes, and go test -race passes on config, scheduler, purchase and api.
• go test -tags integration passes on config and scheduler (testcontainers Postgres).
• pre-commit passes on the changed files.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 4 billable files and costs up to $1.00.}

• 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 9 minutes for your next included review.

Check out review usage here.

View limit details

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

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/suppression-legacy-shapes
• File: .coderabbit.yaml
• Commit: f66947f
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: 4fc24f77-87d0-4a9f-aa4d-8c0468853476

Commits

Reviewing files that changed from the base of the PR and between a4c0756 and f66947f.

Files selected for processing: 4

• internal/config/recommendation_identity.go
• internal/config/suppressions_build_test.go
• internal/config/types.go
• internal/scheduler/scheduler_suppressions_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]

Independent review 1 (cold, fresh context):

VERDICT: APPROVE f66947f

Findings (no blockers):

• nit internal/config/types.go:801 - AppliesTo canonicalises tenancy for every provider/service, while SuppressionVariant (recommendation_identity.go:79) scopes it to AWS EC2. The two disagree: the "non-EC2 untouched" test pins an RDS variant tenancy "shared", yet AppliesTo would equate it with "default". Not reachable today: only common.ComputeDetails carries a tenancy key (pkg common/types.go:558), Azure compute never sets it (azure services/compute/client.go:755), GCP has none, and rows are grouped by provider+service first (scheduler.go aggregateSuppressions). Scoping would need Provider/Service, which AppliesTo has on the row (s.Provider, s.Service), so it is cheap if wanted.
• nit internal/config/recommendation_identity.go:79-81 - with AppliesTo normalising, the write-side tenancy canon is redundant for matching (probe M2: only the config build test fails, the scheduler end-to-end test still passes). It only merges "shared" and "default" buckets within one request and normalises the stored value. Harmless, so keep or drop.
• nit internal/config/recommendation_identity.go:107 - detailsAvailabilityZone does not TrimSpace, unlike zonalAvailabilityZone (scheduler/zonal_recs.go:14) and recDetailsAvailabilityZone (api/handler_purchases.go:3515). Both sides go through the same function, so matching stays consistent. Cosmetic.
• nit (residual, not a defect of this diff) - rows already in purchase_suppressions with AZ "" (written 2026-10-23 onward from a field-less legacy rec) or tenancy "shared" are healed only for tenancy (AppliesTo). Stored AZ "" rows still miss the zonal rec until they expire at the grace period. Low exposure: variant columns exist only since fix(purchase): scope purchase suppressions to the AZ, platform and tenancy bought #1110 (2026-10-23).
• nit - exact-match "shared": a title-cased legacy "Shared" would not be canonicalised. The ec2 client lowercases (aws services/ec2/client.go:322). I found no evidence that "Shared" was ever stored (the platform/go history before 2026-06 has no parser that stores CE values verbatim), so this is speculative.

Attacked:

• Reachability: RecommendationRecord.AvailabilityZone is the top-level payload field (types.go:628), stamped from details by the scheduler (scheduler.go:1733). Migration 000109 (2026-10-12, feat(db): add generated availability_zone column and 9-column recommendations index #808) falls back to details.availability_zone for rows without the field, so the Go fallback now mirrors the DB generated column. The key availability_zone matches common.ComputeDetails (pkg common/types.go:560). "shared" is confirmed as the legacy spelling: ec2 client.go:318-324 ("legacy 'shared' written before fix"), and the parser maps CE shared to default (recommendations/parser_services.go:101-103).
• AZ fallback narrowing: no. The fallback fires only when the field is "". Modern collection sets field = trimmed details AZ, so it is a no-op for regional and modern recs. A legacy rec previously produced {"",p,t}, which never matched a zonal rec anyway, because AZ is not a wildcard. The unknownVariant wildcard (suppressions_build.go:47) checks platform and tenancy only, so it is unaffected. The write side (BuildSuppressions) and the read side (scheduler matchingSuppressions, scheduler.go:1435) both call SuppressionVariant, so the fallback is shared.
• Bypasses: grep for variant/tenancy/AZ comparisons. ListActiveSuppressions has no SQL filter on the variant (store_postgres_suppressions.go:160-167), and AppliesTo is the only matcher. RecIdentityKey and the scheduler rec ID use the raw tenancy, but they are rec identity, not suppression matching, so out of scope.
• Unit tests: go test -race -count=1 ./internal/config/ ./internal/scheduler/ passed. Integration tests (-tags integration, under the heavy lock) passed.
• Merge: git merge-tree --write-tree origin/main (a4c0756) f66947f gives a clean tree 47a8d6b7, rc=0.
• Mutation probes (scratch copy):
  M1 drop the AZ fallback: fails TestBuildSuppressions_LegacyShapesCanonicalised and TestApplySuppressions_LegacyEC2Shapes.
  M2 drop the SuppressionVariant tenancy canon: fails only the config build test (the scheduler test passes, see nit).
  M3 drop the AppliesTo canon: fails AppliesTo_LegacySharedTenancy and LegacyEC2Shapes.
  M4 guard set to if true: fails LegacyShapesCanonicalised ("non-EC2 untouched").
  M5 canonical func made identity: fails all three new tests.
  M6 details AZ always overrides the field: fails 4 config tests.
  Restored; both files cmp byte-identical to the PR SHA.
• Six dimensions: complete for both shapes named in the 2026-10-25 comment on fix(purchase): close two narrow double-buy windows on finalize and legacy retries #1232, and correct. Security: no change. Bugs: only the nits above. No duplication beyond the existing two Details-AZ readers. Not over-engineered.

Plan: nit 1 (scope the AppliesTo tenancy canon to AWS EC2 via the row's Provider/Service) and the TrimSpace nit go into a follow-up commit on this branch after the second review, so both reviews see one more head.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE at f66947f

Blocking findings: none.
Non-blocking findings:
N1 internal/config/types.go:800-802. AppliesTo maps tenancy for every provider and service, but SuppressionVariant only does it for AWS EC2 (recommendation_identity.go:79). The "non-EC2 untouched" test (suppressions_build_test.go, rds case) keeps tenancy "shared", and AppliesTo would then match that row to "default". No pinned producer ever writes a non-EC2 tenancy (see the table), so nothing is affected today. PurchaseSuppression carries Provider and Service, so AppliesTo can be scoped the same way. Probe P3 (scoping it to EC2) fails only the provider-less unit test. The scheduler test still passes.
N2 types.go:801. The rec-side (b) normalisation is redundant: SuppressionVariant already canonicalises every rec that reaches AppliesTo (scheduler.go:1435). Probe P1 (normalise only the stored side) passes every test. It is harmless, but the "rec shared vs stored default" direction is untested.
N3 recommendation_identity.go:91-96 duplicates common.NormalizeTenancyName (pkg@ff6225d common/engine.go:98). The copy is case-sensitive and does not trim whitespace. config already imports common (suppressions_build.go:6). The common helper maps unknown values to "" and so is not an exact fit, but the copy should fold case and trim, or explain why it doesn't. A legacy "Shared" or " shared" would still miss. Probe P11 (case fold) passes, so case handling is untested. I found no evidence that legacy rows are title-case: the repo history is squashed and the pinned CE parser emits lowercase "default".
N4 recommendation_identity.go:67-72. The doc comment still says SuppressionVariant reads "the same AZ field and the same Details read as RecIdentityKey" and "cannot drift from the identity key". It now drifts on purpose (tenancy and AZ). Update the comment.
N5 recommendation_identity.go:98-107. A non-string details.availability_zone silently yields "", and ComputeIdentity does not report it because it decodes only platform and tenancy. So the comment "ComputeIdentity has already reported a decode error" is false for that case. There is also no TrimSpace, unlike zonalAvailabilityZone (scheduler/zonal_recs.go:14). Only low-likelihood inputs hit this.
N6 Not fixable by this PR, so no change requested: suppression rows already stored with AZ "" that came from Details-only recs still miss zonal recs after deploy. The read side cannot tell them apart from genuine regional rows. They expire after the grace period. The az_config gap from #1239 is still open (RDS single-az vs multi-az is not part of the variant), and this PR leaves it unchanged.
Tenancy table (provider / service / values / shared==default? / effect / test):

• AWS / EC2 (ec2|compute) / default, dedicated (CE "Shared" goes to "default", aws parser_services.go:97-115; ec2 client.go:327-336 accepts shared or default) / yes, the AWS API treats "shared" as an alias of "default" / matched on both the write and read sides / tested.
• AWS / RDS, ElastiCache, Redshift, OpenSearch, MemoryDB / no tenancy key (only ComputeDetails has json "tenancy", common/types.go:596) / n/a / no-op / only the rds "untouched" case.
• Azure / VM / Tenancy never set (azure compute/client.go:759-769). Azure's "Shared" is appliedScopeType, which never reaches details.tenancy / n/a / no-op / untested.
• GCP / Compute Engine, sole-tenant / Tenancy never set (gcp commitment_type.go:118, client.go:1388) / n/a / no-op / untested.
• "dedicated" vs "host" stay distinct (only the exact string "shared" is mapped). Probe P4 confirms this.
  AZ analysis: in the AWS parser, Details.availability_zone is set only when CE returns an AZ, which implies zonal scope (parser_services.go:117-121, 170-173). The rec field is stamped from that same value (scheduler.go:1733). The fallback runs only when the field is empty (recommendation_identity.go:81), and the field wins when both are set (probe P7 is caught). The fallback narrows a falsely regional variant to its real zone and never widens it. Regional and zonal stay distinct, and the AZ is still compared (P5 is caught). The EC2 wildcard (suppressions_build.go:47-52) checks only platform and tenancy. Neither change makes either of them empty, and empty Details gives "" through an Unmarshal error. So the two wildcards cannot combine into a match-all.
  AppliesTo callers: only scheduler.go:1440 (matchingSuppressions). SuppressionVariant callers: suppressions_build.go:40 and scheduler.go:1435. RecIdentityKey, plan matching, purchase dedupe and pricing do not call either, so nothing leaks into those paths. There is no SQL-side matching (store_postgres_suppressions.go only inserts, lists and deletes).
  Consumer table: aggregateSuppressions (scheduler.go:1414) is unchanged and uses CanonicalSuppressionService. applySuppressionIndex and matchingSuppressions take the new behaviour through AppliesTo. The BuildSuppressions writers are plan_step_attach.go:348, attach_recommendations.go:35, and handler_purchases.go:2608 and :3618; all of them get canonical tenancy and the Details AZ through suppressionBucketOf. Store write and read (store_postgres_suppressions.go:110, 185) are unchanged. There are no new consumers. Legacy stored "shared" rows are handled on the read side by AppliesTo, as N6 notes.
  Probes (scratch edits in the head export, each restored with cp and checked with cmp; I also confirmed both files match the SHA via git show):
  P1 normalise only the stored side: all tests pass (gap, N2). P1b normalise only the rec side: caught.
  P2 drop the "shared" mapping: caught (config and scheduler). P3 scope AppliesTo to EC2: only the provider-less config unit test fails (gap, N1).
  P4 map dedicated to default: caught. P5 ignore the AZ in AppliesTo: caught. P6 drop the Details-only branch: caught.
  P7 Details beats the field: caught. P8 apply to all services: caught (rds case). P9 drop the SuppressionVariant tenancy step: config test fails, scheduler passes, which shows the step is redundant for matching.
  P10 drop the aws provider check: all tests pass (gap: Azure or GCP "compute" would get the AZ and tenancy steps, a no-op today). P11 case-fold "shared": passes (case handling untested).
  Commands (env: no AWS config or creds, ARM/AZURE/GOOGLE/CLOUDSDK vars unset, GOTOOLCHAIN=go1.26.10):
• go build ./...: ok.
• go vet on config and scheduler, with and without -tags integration: ok.
• gofmt -l: clean.
• go test -count=1 config scheduler: ok.
• Fail-before (head tests copied into a copy of the base export): all 3 new tests fail (config: 3 subtests plus the AppliesTo test; scheduler: 4 of 4 subtests).
• golangci-lint run on config and scheduler: 1 prealloc hit in the untouched store_postgres_history_page_integration_test.go:134, which predates this PR.
• nice -n 10 lockf -k -t 3600 /tmp/claude/heavy.lock go test -tags integration on config and scheduler: ok (39.6s and 8.6s).
• git merge-tree --write-tree origin/main(a4c0756) f66947f: clean (tree 47a8d6b). No drift in the touched files since base bee0627.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

Both reviews approve f66947f, so this merges at that SHA. The shared review points go into a follow-up PR: scope the AppliesTo tenancy canonicalisation to AWS EC2, reuse common.NormalizeTenancyName instead of the local normaliser, test the stored-side-only direction, and fix the stale doc comment and the non-string Details AZ handling.

[cristim]

Local verification at f66947ffc118111d12644b273f26753412922ef0 merged onto current main (b9b0dafb; merge-tree 4ba2c154; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• go test -tags integration passes on real Postgres (testcontainers) for the touched packages: ./internal/config ./internal/scheduler
• Nothing was applied to any cloud.