[cristim]

Closes #1420.

Problem: #1412 (#1419) made purchase suppressions match AWS ec2 and compute as the same service, but the other AWS legacy slugs were still compared literally. Rec rows persisted before slug normalisation still carry rds, elasticache, opensearch and redshift; that is why mapServiceSlug in internal/purchase/execution.go keeps them. A retried legacy execution writes its suppression under the legacy slug. Current recs use relational-db, cache, search and data-warehouse, so the scheduler did not hide the rec that had just been bought, and it could be bought again.

Change: config.CanonicalSuppressionService now maps all five AWS legacy slugs to their canonical spelling through one map. memorydb has no canonical slug, and non-AWS providers pass through unchanged. Every existing call site already goes through this function (suppressionBucketOf, aggregateSuppressions, applySuppressionIndex), so no caller changes.

Tests:

• TestCanonicalSuppressionService is a 17-row table. It covers every legacy pair, idempotence on canonical input, memorydb passthrough, and Azure/GCP passthrough.
• TestApplySuppressions_EC2SlugSpellings gains 10 rows: legacy suppression vs canonical rec and the reverse for each pair, memorydb not hiding a cache rec, and Azure rds not hiding a relational-db rec.
• Before the change, 4 config and 8 scheduler subtests fail. Dropping the redshift map entry fails the config row and 2 scheduler rows.

Local:

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

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • AWS suppression rules now recognize legacy and canonical service names for EC2, RDS, ElastiCache, OpenSearch, and Redshift, so equivalent labels match consistently.
  • Recommendations using both rds and relational-db for the same service are combined into one suppression, with their counts summed.
  • Matching remains provider-specific: Azure service names and unrelated AWS service names are not treated as equivalent.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/suppression-aws-aliases
• File: .coderabbit.yaml
• Commit: 6bfbc19
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: 66d85a26-1307-4fee-bfb4-220144e4b482

Commits

Reviewing files that changed from the base of the PR and between 9b6e470 and 6bfbc19.

Files selected for processing: 4

• internal/config/suppressions_build_test.go
• internal/config/types.go
• internal/purchase/execution.go
• internal/scheduler/scheduler_suppressions_test.go

Files skipped from review as they are similar to previous changes: 1

• internal/config/types.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 2 reviews per hour.

Walkthrough

AWS suppression canonicalization now maps five legacy service slugs to canonical slugs. Tests cover canonicalization, aggregation of RDS aliases, and suppression matching across AWS service spellings.

Changes: AWS suppression aliases

Layer / File(s)	Summary
Canonicalization and suppression matching internal/config/types.go, internal/config/suppressions_build_test.go, internal/scheduler/scheduler_suppressions_test.go, internal/purchase/execution.go	CanonicalSuppressionService maps ec2, rds, elasticache, opensearch, and redshift to canonical service slugs for AWS. Tests check idempotence, merged RDS suppression counts, matching in both directions, and distinct MemoryDB and Azure services. A comment directs contributors to update the canonicalization map when adding legacy AWS-only aliases.

Priority: Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Possibly related issues

• fix(config): purchase suppressions miss recs stored under the other EC2 service slug #1412: This PR retains EC2 alias canonicalization and its suppression matching behavior.

Possibly related PRs

• LeanerCloud/cloud-commitments-platform#1419: This PR extends the CanonicalSuppressionService added in PR #1419.

Suggested labels: impact/internal, priority/p3, severity/low, urgency/this-quarter, effort/xs

Suggested reviewers: iamjsd

Merge Risk: Minimal · up to 6bfbc

No concrete merge-blocking risk was found in the reviewed change.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title names the main change: canonicalizing AWS service aliases in purchase suppressions, in the config package. The raw summary and objectives show the PR extends CanonicalSuppressionService from…
Linked Issues check		Issue #1420 requires AWS alias pairs rds/relational-db, elasticache/cache, opensearch/search, and redshift/data-warehouse to canonicalise in purchase suppressions, with tests in both directions and un…
Out of Scope Changes check		The whole-PR diff touches internal/config/types.go (suppression canonicalisation map and function), internal/purchase/execution.go (comment only), and related tests in internal/config and internal/sch…
Docstring Coverage		Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 4 files.

Full details: Title check

Explanation

The title names the main change: canonicalizing AWS service aliases in purchase suppressions, in the config package. The raw summary and objectives show the PR extends CanonicalSuppressionService from ec2 only to ec2, rds, elasticache, opensearch, and redshift. The title says "every AWS service alias", which is slightly broader than the five slugs covered, but the PR objectives describe the same scope. The title is specific and matches the changeset, so it passes.

Full details: Linked Issues check

Explanation

Issue #1420 requires AWS alias pairs rds/relational-db, elasticache/cache, opensearch/search, and redshift/data-warehouse to canonicalise in purchase suppressions, with tests in both directions and unchanged Azure/GCP behaviour. The whole-PR diff of internal/config/types.go (merge base to head) shows the awsLegacyServiceSlugs map with ec2->compute, rds->relational-db, elasticache->cache, opensearch->search, and redshift->data-warehouse. CanonicalSuppressionService returns the input unchanged when provider is not aws. It also returns unchanged any service absent from the map, so memorydb passes through. The whole-PR diff of internal/purchase/execution.go changes only a comment that cross-references the new map, so mapServiceSlug behaviour is unchanged. The changes since the previous review (comment cross-references, test rename, and a BuildSuppressions merge test) do not alter the mapping logic. The incremental diff against the previous reviewed head was unavailable, so the test-file changes were confirmed only from the supplied summary. The previous PASS conclusion is still supported by the whole-PR code evidence. The issue's suggestion to derive the map from mapServiceSlug was an ideal, not a requirement; the explicit map with a cross-reference comment is accepted.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff touches internal/config/types.go (suppression canonicalisation map and function), internal/purchase/execution.go (comment only), and related tests in internal/config and internal/scheduler. All code changes directly implement the #1420 alias canonicalisation and its non-AWS and memorydb passthrough behaviour. The changes since the previous review are comments, a test rename, and an added BuildSuppressions merge test for rds/relational-db, which serve the same objective. No unrelated product behaviour is present in the inspected evidence. The incremental test-file diff could not be read, so its content rests on the supplied summary.

Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• 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 9b6e470

Findings (no blocking issues):
nit internal/config/types.go:766-772 - the alias table duplicates the legacy half of mapServiceSlug (internal/purchase/execution.go:1294-1309); the issue asked for one shared table. config cannot import purchase, so the copy is defensible, but neither side references the other - a new AWS alias added to one will silently miss the other (double-buy class returns). Suggest a cross-reference comment, or have mapServiceSlug derive its legacy keys from config.
nit internal/scheduler/scheduler_suppressions_test.go:366 - TestApplySuppressions_EC2SlugSpellings now covers every AWS alias; the name is stale.
nit internal/config/suppressions_build_test.go - no BuildSuppressions case for a non-EC2 alias pair (e.g. "rds" + "relational-db" in one request merging into one canonical row). Removing the rds map entry is still caught by TestCanonicalSuppressionService and the scheduler tests, so coverage is adequate; the write-side merge is untested.

Attacked:

• Reachability: the pinned providers/aws module emits canonical slugs for RDS/ElastiCache/OpenSearch/Redshift (services/rds/client.go:105, elasticache/client.go:98, opensearch/client.go:113, redshift/client.go:117) but legacy "ec2" for EC2 (ec2/client.go:108). The scheduler persists string(rec.Service) unchanged (scheduler.go:1728). API purchase recs keep the caller's rec.Service (validation.go:623 only trims), and mapServiceSlug accepts both spellings (execution.go:1296-1308), so a request sent under a legacy slug against a canonical scheduler rec (or an older persisted row) is a real mismatch.
• Completeness: CanonicalSuppressionService is applied on the write side (suppressions_build.go:35, so rows are now stored canonical) and on both read-side keys (scheduler.go:1420, 1466). ListActiveSuppressions (store_postgres_suppressions.go:159) is the only reader, and the scheduler is its only consumer; no SQL filters suppressions by service (the DELETEs key on execution_id). RecIdentityKey (recommendation_identity.go:52) uses raw Service, but it is a pricing identity, not suppression: out of scope.
• Slug correctness against cc-go pkg/common/types.go:84-145: compute/relational-db/cache/search/data-warehouse match. The provider treats the same pairs as equivalent (providers/aws provider.go GetServiceClient switch).
• memorydb: it has its own client and ServiceMemoryDB recs (memorydb/client.go:93), distinct from ElastiCache, so leaving it unmapped is correct. Mapping it to cache would hide ElastiCache recs (over-suppression).
• Over-suppression: the provider guard confines the mapping to AWS. Azure/GCP slugs pass through, and the azure rds/relational-db and gcp compute/ec2 cases stay distinct. Within AWS every pair is 1:1, so no two distinct services collapse together.
• Persisted legacy rows: both read keys are canonicalised, so old "rds" suppressions still match new "relational-db" recs. Merging aliases in one request removes, rather than adds, UNIQUE-constraint risk.
• Ran: go test -race ./internal/config ./internal/scheduler ok; -tags integration (under lockf) ok; go vet ok; gofmt clean.
• Mutation probes (scratch copy, restored and confirmed with cmp):
  remove "rds" entry -> 3 subtests fail (canonical + both scheduler directions)
  remove "elasticache" -> 3 fail; remove "opensearch" -> 3 fail; remove "redshift" -> 3 fail
  remove "ec2" -> 11 FAIL lines (including BuildSuppressions legacy-EC2 wildcard tests)
  remove the provider != "aws" guard -> 9 FAIL lines (azure/ec2, azure/rds, gcp/redshift, ...)
  add "memorydb":"cache" -> canonical/aws/memorydb and the memorydb-does-not-hide-cache scheduler case fail

Nits 1-3 will land in a follow-up commit on this branch; that commit gets a delta review.

[cristim]

Independent review 1, delta (same cold reviewer):

VERDICT: APPROVE 6bfbc19

Delta 9b6e470..6bfbc19 (one commit, 4 files, +19/-4). No new findings.

• Nit 1 (alias table duplication), resolved: comments in both directions now link the tables, at internal/config/types.go:765-766 and internal/purchase/execution.go:1302-1303. The only Go change is a comment, so behaviour is the same.
• Nit 2 (stale test name), resolved: the test is renamed to TestApplySuppressions_AWSLegacySlugSpellings and no reference to EC2SlugSpellings is left under internal/.
• Nit 3 (write-side merge untested), resolved: TestBuildSuppressions_MergesRDSAliases checks that "rds"(3) and "relational-db"(2) produce one row with "relational-db" and count 5.
  Attacked:
• On an archive of 6bfbc19, go test -race -count=1 passed for ./internal/config/, ./internal/scheduler/ and ./internal/purchase/. gofmt is clean.
• Mutation probe: dropping the "rds" map entry makes TestBuildSuppressions_MergesRDSAliases FAIL ("should have 1 item(s), but has 2": separate rds and relational-db rows). After restoring the file (confirmed with cmp) it passes again.

[cristim]

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

Blocking items: none.

Non-blocking items

1. internal/config/types.go:767 copies the alias table by hand, using string literals. internal/purchase/execution.go:1302 and types.go:763 now point at each other in comments, but no test ties the map to mapServiceSlug or to the provider's GetServiceClient. Nothing fits well enough to reuse: mapServiceSlug returns legacy common constants, not canonical ones, and the pinned common package has no legacy-to-canonical helper. Keying the map with common.ServiceRDS / common.ServiceRelationalDB etc. would at least tie it to the library constants. A drift test needs an exported accessor, which is optional.
2. Savings Plans spelling aliases are not covered: "savings-plans" and "savingsplans", and "savingsplans-compute" and "savings-plans-compute" (execution.go:1337-1349). The frontend sends only the "savings-plans-*" spelling (frontend/src/settings.ts:137-140), and the pre-sec(auth): MFA lifecycle endpoints are unthrottled (unlimited online TOTP brute force) #94 retention window ends 2026-10-30. That is outside fix(config): canonicalise every AWS service alias in purchase suppressions, not only ec2/compute #1420's scope and hard to reach, so consider a follow-up issue. Do not collapse the umbrella "savingsplans" into the per-plan-type slugs: they are different services, and doing so would over-suppress.
3. Service matching is case-sensitive (types.go:785). It matches mapServiceSlug, and validation.go:623 only trims the service. An "RDS" purchase gets "unsupported service" from the provider instead of buying, so case does not open a double buy.
4. golangci-lint reports a prealloc finding in internal/config/store_postgres_history_page_integration_test.go:134. The PR does not touch that file, so it predates this PR.

Pair table (taken from pinned providers/aws@v0.0.0-20261020071945-92722e673741 provider.go:468-477 GetServiceClient and :434-453 GetSupportedServices)
| pair | in provider | in PR | canonical | test |
| ec2 / compute | yes | yes (from #1419) | compute | config table + scheduler rows, both directions |
| rds / relational-db | yes | yes | relational-db | config + scheduler both directions + MergesRDSAliases |
| elasticache / cache | yes | yes | cache | config + scheduler both directions |
| opensearch / search | yes | yes | search | config + scheduler both directions |
| redshift / data-warehouse | yes | yes | data-warehouse | config + scheduler both directions |
| memorydb | separate client, no alias | correctly absent | memorydb | "memorydb sup does not hide cache rec" |
| SP slugs | per-plan-type cases, no legacy pair in the provider | absent | n/a | see non-blocking item 2 |

The PR adds no extra pairs. Only AWS is affected (types.go:782); Azure and GCP pass through, and tests cover that.

On the canonical choice: the pinned AWS service clients emit the canonical slug for RDS, ElastiCache, OpenSearch and Redshift (for example services/rds/client.go:105 and redshift/client.go:92). EC2 still emits "ec2" (ec2/client.go:109), and #1419 already handles it. BuildSuppressions now writes the canonical slug to new rows (suppressions_build.go:35, the Service field). Legacy rows stored under either spelling still match, because aggregateSuppressions canonicalises the stored value on read (scheduler.go:1420), so no migration is needed. The #1411 empty-Details wildcard (suppressions_build.go:40-44) and the #1232 EC2 wildcard (:46-52) do not depend on the service spelling for the new pairs. The only reader is ListActiveSuppressions (store_postgres_suppressions.go:159), which reaches the scheduler only. No SQL filters on service: only execution_id deletes (store_postgres.go:909, store_postgres_suppressions.go:147).

Consumer table at head

• scheduler aggregateSuppressions, scheduler.go:1420: canonicalises.
• applySuppressionIndex, scheduler.go:1466: canonicalises.
• BuildSuppressions bucket, suppressions_build.go:35: canonicalises.
• fix(purchase): suppress every variant when a legacy EC2 recommendation has no details #1411 wildcard, suppressions_build.go:40: uses the canonical bucket.
• Writers via BuildSuppressions: attach_recommendations.go:35, plan_step_attach.go:348, handler_purchases.go:2602 and :3612 (the line was :3585 in fix(config): match purchase suppressions across the ec2 and compute slugs #1419's table).
  No new callers of CanonicalSuppressionService or new readers of suppressions since fix(config): match purchase suppressions across the ec2 and compute slugs #1419.

Probes (scratch export copy; each restored with cp, then cmp OK)
At 9b6e470:

• drop rds: config row and 2 scheduler rows fail.
• drop opensearch: config row and 2 scheduler rows fail.
• map elasticache to memorydb (wrong canonical): config row and 2 scheduler rows fail.
• remove the provider check: azure/gcp config rows and 3 scheduler rows fail.
• add memorydb to cache (collapse an unrelated service): memorydb config and scheduler rows fail.
• canonicalise the rec side only (sup raw at scheduler.go:1420): 9 tests fail.
• canonicalise the sup side only: 11 tests fail.
• map redshift to compute (cross-service collapse): redshift rows fail.
• write side canonicalises ec2 only: passed at 9b6e470 (gap). At 6bfbc19 TestBuildSuppressions_MergesRDSAliases now fails, so the gap is closed.
  At 6bfbc19 I re-ran: drop redshift, rds mapped to rds, remove the provider check, write side ec2 only. All fail as expected.

Delta 9b6e470 to 6bfbc19

• One commit; its parent is 9b6e470.
• It adds 2 comment lines each in types.go and execution.go, renames the scheduler test with no change to its body, and adds TestBuildSuppressions_MergesRDSAliases.
• This matches the author's claim. The new test fails on base: 2 rows instead of 1.

Commands and results

• git fetch +pull/1421/head: rev-parse gives 6bfbc19.
• go build ./...: OK at both heads.
• go vet on config, scheduler and purchase, with and without -tags integration: OK.
• gofmt -l: clean.
• go test -count=1 on config, scheduler and purchase: ok.
• Fail-before (head tests copied onto base, base files restored, cmp OK): 4 config subtests and 8 scheduler subtests fail at 9b6e470. At 6bfbc19 the MergesRDSAliases test fails as well.
• golangci-lint on the touched packages: 1 finding, in an untouched file (see non-blocking item 4).
• go test -tags integration (testcontainers) on config and scheduler, run under the heavy lock: ok (36s and 13s).
• git merge-tree --write-tree origin/main (f316add) 6bfbc19: clean, tree 4a965a7a. origin/main has no changes in the touched paths since base 9cdeb78.

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

[cristim]

Local verification at 6bfbc199bdc66f64922526e9c3e200c68dc4894b merged onto current main (f316add3; merge-tree 4a965a7a; 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/purchase ./internal/scheduler
• Nothing was applied to any cloud.