[IAmJSD]

Root cause

The purchase suppression key was the 6-tuple (account, provider, service, region, resource type, engine). BuildSuppressions wrote one row per 6-tuple, and aggregateSuppressions / applySuppressionIndex in internal/scheduler/scheduler.go subtracted that row from every rec on the tuple. Buying us-east-1a Linux/default therefore also cut the us-east-1b, regional, Windows and dedicated recs, and hid them for the whole grace window. The rec ID, identity key and recommendations natural key already carry AZ, platform and tenancy (#792, #806); only the suppression key did not.

Fix

• Migration 000134 (expand step, safe for old code): adds nullable availability_zone, platform and tenancy columns to purchase_suppressions, with no default, so there is no table rewrite. A CHECK keeps them all-NULL or all-set. The 6-tuple UNIQUE is replaced by UNIQUE NULLS NOT DISTINCT (execution_id, 6-tuple, availability_zone, platform, tenancy). Old code still works on the migrated schema: its inserts name only the 6-tuple, so the new columns are NULL; it adds up its counts per 6-tuple before inserting; and it never uses the old constraint (plain INSERT, no ON CONFLICT). Down merges rows of one execution that differ only by variant into one row per execution and 6-tuple. The merged row keeps the summed count and the latest expiry. Down then drops the columns and restores the old constraint.
• Write side: BuildSuppressions writes one row per 6-tuple plus variant. The AZ comes from rec.AvailabilityZone, and platform and tenancy come from ComputeIdentity(). These are the same reads RecIdentityKey uses, through the new RecommendationRecord.SuppressionVariant(). Empty strings are known values (a regional AZ, or a non-EC2 rec), not wildcards.
• Read side: suppressions are grouped by 6-tuple. A row counts against a rec only when PurchaseSuppression.AppliesTo(recVariant) is true. That means an exact variant match, or a row with no variant.
• Legacy wildcard (the requirement noted in feat(planstep): resolve plan steps to sized, priced recommendations (not wired) #822/feat(purchase): attach resolved recommendations to a plan step under the provider lock #836): a row with NULL variant columns still applies to every rec on its 6-tuple, as before. Such rows were written before 000134, or by old instances during a rolling deploy. The same rule covers a rec whose Details cannot be read: on either side it falls back to 6-tuple matching. So the fallback is always the old behavior, which hides too much and under-buys, rather than showing capacity that was just bought. The purchase paths already reject unreadable Details through RecIdentityKey, so this fallback is defensive.
• No change to the purchase guard: computeDetailMismatch already compares platform and tenancy.

Closes #816

Deploy / runbook

• Prod runs with auto_migrate=false. Apply 000134 before releasing this code. The migration is compatible with old code. New code on the old schema fails every suppression insert (column "availability_zone" does not exist), which rolls back the purchase or attach transaction. It fails closed (nothing is bought) but it does fail. Use database-migration.yml with steps set to the true pending count. Dev auto-migrates at startup before the stores are rebuilt.
• Rolling deploy: old instances read the new rows as 6-tuple rows, and new instances read old rows as wildcards. Both over-suppress, which is safe. Rows from the old code age out within one grace window (7 days by default).
• Rollback: rolling back the code alone is safe because old code ignores the columns. Down 000134 is only needed to drop the schema change, and it merges variant rows as described above.
• Concurrency: suppression rows are insert-only, written in the transaction of their own execution, and keyed by execution_id. Subtraction is computed at read time, so there is no read-modify-write and no cross-execution conflict. The existing concurrent integration tests (TestAttachIntegration_PlansOnOneAccountNeverOverAttach, TestAttachIntegration_ConcurrentTicksOnOnePlanMintOneRow, TestAttachResolvedRecommendations_ConcurrentAttachesOnce) pass on real Postgres 16 with the migration applied. I added no new race test because the change creates no new shared state.

Verification

All tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's default AWS profile is invalid. Integration tests ran against a throwaway postgres:16-alpine testcontainer, through the real migration chain and the real store. No shared or deployed database was touched.

Fail-before / pass-after (production files reset to origin/main 8caed39 and 000134 removed; tests unchanged):

• Unit: go test ./internal/scheduler/ -run TestApplySuppressions_OnlyThePurchasedVariant fails on main. other-az, regional, windows and dedicated each show count 2 instead of 5, with SuppressedCount 3 and a badge. It passes with the fix.
• Real Postgres: go test -tags integration ./internal/scheduler/ -run TestSuppressionsScopedToPurchasedVariant_RealPostgres fails on main with {"bought":2, "other-az":2, "regional":2, "windows":2, "dedicated":2, ...}. The fix gives {"bought":2, "other-az":5, "regional":5, "windows":5, "dedicated":5, "legacy-a":4, "legacy-b":4}. The legacy pair is a row inserted the way old code does it (no variant columns), and it still applies to both variants.

New tests

• internal/config: TestBuildSuppressions_SeparatesVariants, _NonComputeVariantIsKnownEmpty, _UnreadableDetailsFallBackToWildcard, TestPurchaseSuppression_AppliesTo; pgxmock TestPGXMock_CreateSuppression_WritesVariant. The existing round-trip and list tests now cover the NULL and set columns.
• internal/scheduler: TestApplySuppressions_OnlyThePurchasedVariant, _LegacyRowIsAWildcard, _NonComputeVariantMatches, _UnreadableRecDetailsMatchEveryRow, plus the real-Postgres test above.
• internal/database/postgres/migrations: TestMigration_PurchaseSuppressionsVariant (uses previousMigrationVersion). It covers a pre-migration row reading back as NULL, old-code inserts still working, a legacy duplicate still rejected (NULLS NOT DISTINCT), five variant rows coexisting in one execution, an exact duplicate rejected, the CHECK rejecting a partial NULL, down merging into one row with the summed count and restoring the 6-tuple constraint, and up re-applying after down.

Mutation probes (all 10 killed): match always true; variant never written; insert writes NULL; scan drops the variant; tenancy or AZ dropped from the variant; legacy rows not treated as wildcards; unreadable rec variant matching nothing; bucket ignoring the variant; earliest-expiry update disabled.

Commands run on the final tree

go build ./... && go vet ./...
go test ./internal/config/ ./internal/scheduler/ ./internal/planstep/ ./internal/purchase/ ./internal/api/ ./internal/server/ ./internal/mocks/ -count=1   # all ok
go test -short -race ./internal/config/ ./internal/scheduler/ -count=1                                                                                    # ok
go test -tags integration ./internal/database/postgres/migrations/ -count=1                                                                               # ok (whole package)
go test -tags integration ./internal/scheduler/ -count=1                                                                                                  # ok (whole package)
go test -tags integration ./internal/purchase/ -run 'TestPlanProviderLock_|TestManualPlanStep_|TestAttachIntegration_|TestCumulativeAttach_|TestFailExistingPlanStep_|TestCrash_|TestGetOrCreateExecution_|TestAttachResolvedRecommendations_|TestExpireExecutionAtomic_|TestSetExecutionRecommendationsIfEmpty_' -count=1   # ok
go test -tags integration ./internal/api/ -run 'TestCombinedPlanPurchase_|TestNotifiedRow_' -count=1                                                      # ok
go test -tags integration ./internal/config/ -run 'TestDeletePurchasePlan_CancelsOpenStepRows|Suppress' -count=1                                          # ok
golangci-lint v2.10.1 run --build-tags integration ./internal/config/... ./internal/scheduler/... ./internal/database/postgres/migrations/...            # 0 issues
gocyclo -over 10 -ignore "_test\.go" <changed files>                                                                                                      # silent
scripts/gofmt-hook.sh, make tidy-check, scripts/gosec-hook.sh (exit 0), migration-number dup check
scripts/test-git-secrets-allowlist.sh (42 PASS) and git secrets --scan in an isolated fresh repo                                                          # clean

Evidence and gaps: this is unit, pgxmock and real-local-Postgres integration evidence on synthetic fixtures. It does not include a real AWS collection or purchase. I did not apply the migration to any deployed database.

Out of scope

• idx_purchase_suppressions_lookup stays on the 6-tuple plus expires_at. The list query filters only on expires_at, and grouping still uses the 6-tuple, so this PR does not need a wider index.
• No frontend change. The badge fields (suppressed_count, suppression_expires_at, primary_suppression_execution_id) keep their shape and are now set per variant.
• Extend recommendation storage/scheduler identity to include purchase-critical Details (tenancy/platform/scope/az_config) #408 (wider identity work) is not touched.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, urgency/this-quarter, impact/few, effort/m, type/bug, triaged.

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 8caed39
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: f17a5143-2fdd-4cce-a184-c3b49c986eff

📥 Commits

Reviewing files that changed from the base of the PR and between 8caed39 and f127e3f.

📒 Files selected for processing: 12

• internal/config/recommendation_identity.go
• internal/config/store_postgres_suppressions.go
• internal/config/store_postgres_suppressions_test.go
• internal/config/suppressions_build.go
• internal/config/suppressions_build_test.go
• internal/config/types.go
• internal/database/postgres/migrations/000134_purchase_suppressions_variant.down.sql
• internal/database/postgres/migrations/000134_purchase_suppressions_variant.up.sql
• internal/database/postgres/migrations/000134_purchase_suppressions_variant_test.go
• internal/scheduler/scheduler.go
• internal/scheduler/scheduler_suppressions_test.go
• internal/scheduler/suppression_variant_integration_test.go

+---------------------------------------------------------------------------------------------------------------------------+
| If programmers were electricians, parallel programmers would be bomb disposal experts. Both cut wires. - Bartosz Milewski |
+---------------------------------------------------------------------------------------------------------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• 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]

VERDICT: APPROVE f127e3f

Reviewed head f127e3f (base 8caed39). git merge-tree --write-tree origin/main(b6a0649a) f127e3f5 is clean (tree c81afcf7). All tests below ran on that merged tree.

Findings (none blocking):

• nit internal/database/postgres/migrations/000134_purchase_suppressions_variant_test.go:96-111 - the down test checks only the summed count and row count, not the merged row's expires_at or created_at. Probe SQL2 (MAX(expires_at) -> MIN in the down) is expected to survive (result below). A rollback that kept the earliest expiry would end the grace window early for the merged siblings. Add an assert on expires_at.
• nit internal/config/recommendation_identity.go:30-33 + suppressions_build.go - an EC2 rec with empty Details reads as the known variant {az,"",""}, not as unknown. Every purchase path resolves recs by RecIdentityKey, which uses the same read, so today the written variant equals the stored rec's variant. The retry path (handler_purchases.go:2541) rebuilds suppressions from the persisted recs without re-matching. A failed execution persisted before Details existed (fix(db): redact bound query arguments at every log level #453) would write {az,"",""} and leave the stored "Linux/UNIX" rec unsuppressed. That is narrow and legacy only. Treating an EC2 rec with empty Details as unknown (a wildcard) would close it.
• nit 000134 up - ADD CONSTRAINT CHECK and the new UNIQUE take ACCESS EXCLUSIVE and scan the table plus build an index. No rewrite. purchase_suppressions is never purged of expired rows (only execution-scoped deletes), so the table grows with history but stays small. Acceptable.
• process nit: one commit with 786 changed lines (cap 400).

Migration: 000134 is still free on main (latest is 000133), and the open chain uses 135-139 (#1116, #1140, #1170, #1197, #1201). Up and down are symmetric: the down merges per-execution 6-tuple rows (summed count, max expiry) before it restores the original auto-named constraint. The test asserts the constraint name. There is no backfill by design: existing rows stay NULL, act as 6-tuple wildcards and age out within one grace window. That errs toward over-suppressing. NULLS NOT DISTINCT needs PG15+; every env pins 16.

Readers checked: scheduler applySuppressions (ListRecommendations, ListRecommendationsStrict, GetRecommendationByID; planstep/resolve.go uses the suppressed list), and the writers BuildSuppressions at handler_purchases.go:2541/3461, plan_step_attach.go:348 and attach_recommendations.go:35. The write side and the read side share SuppressionVariant(). An unreadable Details falls back to a wildcard on both sides (over-suppresses). No path un-suppresses the purchased variant itself.

Attacked:

• P1 AppliesTo always true -> killed (TestPurchaseSuppression_AppliesTo, TestApplySuppressions_OnlyThePurchasedVariant)
• P2 legacy NULL row not a wildcard -> killed (7 scheduler/config tests)
• P3 scan drops tenancy -> killed (TestPGXMock_ListActiveSuppressions)
• P4 write side drops AZ -> killed (TestBuildSuppressions_SeparatesVariants and scheduler tests)
• SQL1 up drops a wrong old-constraint name -> killed (TestMigration_PurchaseSuppressionsVariant)
• SQL2 down keeps MIN(expires_at) -> SURVIVED (see the nit above)
• SQL3 down keeps MAX count instead of SUM -> killed
• SQL4 insert writes NULL variant -> invalid probe (build failure, unused vars); P3 and the pgxmock write test cover the column mapping

Tests (merged tree, AWS config /dev/null): go build, go vet -tags integration on the touched packages: ok. Unit tests for config, scheduler, planstep, purchase and api: 5932 passed. go test -tags integration ./internal/database/... ./internal/config/... ./internal/purchase/... ./internal/scheduler/...: all ok (run under heavy.lock).

[cristim]

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

Migration order: this PR adds 000134 and is the first link of a chain that must merge strictly in this order:

#1110 (134) -> #1116 (135) -> #1140 (136) -> #1170 (137) -> #1197 (138) -> #1201 (139)

golang-migrate skips a lower version once a higher one is applied.

• origin/main (b6a0649) is at 000133, so 134 is the next free number.
• git merge-tree --write-tree origin/main f127e3f5 is clean.
• Of the touched files, only internal/scheduler/scheduler.go moved on main, in fetchAndConvert, which does not overlap the suppression code.

Findings (none blocking):

• [non-blocking] 000134_purchase_suppressions_variant.down.sql:13 - The "keep the latest expiry" (MAX(expires_at)) is untested. MIN still passes TestMigration_PurchaseSuppressionsVariant, because every test row shares one expiry. The effect would be a shorter suppression window after a rollback.
• [non-blocking] up.sql:30-40 - ADD CHECK and ADD UNIQUE each take ACCESS EXCLUSIVE and scan or build an index. Acceptable for this small table: at 50k rows I measured no table rewrite (relfilenode unchanged).
• [non-blocking] UNIQUE NULLS NOT DISTINCT needs PG15+.
  • Every deployed tfvars file pins 16 (AWS, Azure and GCP).
  • But terraform/environments/azure/variables.tf:214 still accepts "11" through "15", and a PG14 deployment would fail this migration. Worth narrowing that validation.

Against #816:

• The suppression key now carries AZ, platform and tenancy on both sides (suppressions_build.go, scheduler.go matchingSuppressions).
• Both sides read them through SuppressionVariant(), the same reads RecIdentityKey uses.

Attacked:

• Under-suppression (double-buy direction):
  • executePurchase passes request recs, but priceRecommendationsFromStore (internal/api/purchase_pricing.go:33-78) rejects any rec whose RecIdentityKey does not match a stored rec. So the written variant always equals the stored one.
  • The attach and retry paths use stored or persisted recs.
• Fallbacks: legacy NULL rows and recs with unreadable Details fall back to a 6-tuple wildcard. That over-suppresses, which is the safe direction.
• Consumers: only the scheduler reads ListActiveSuppressions, plus a mock.
• Old code against the new schema: it inserts without ON CONFLICT, and the new columns are nullable, so it keeps working.
• Down:
  • The 6-tuple columns are NOT NULL (000037), so the merge's equality joins are safe.
  • Each migration runs in one transaction, so a missing IF NOT EXISTS on the constraints does not matter.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  • M1, AppliesTo always true: caught in config and scheduler.
  • M2, scan drops the variant: TestPGXMock_ListActiveSuppressions FAILS.
  • M3, variant drops tenancy: caught.
  • M4, down without the merge CTE: caught.
  • M5, UNIQUE without NULLS NOT DISTINCT: caught.
  • M6, variant never written: caught.
  • M7, an unreadable rec matches nothing: caught.
  • M8, MIN instead of MAX expiry in down: survives. This is the gap above.

Verified locally, on git archive exports with no cloud credentials, under the heavy lock:

• Head:
  • go build ./...: clean.
  • go vet, with and without -tags integration, on config, scheduler and migrations: clean. gofmt -l: clean.
  • go test -count=1 on config, scheduler, migrations, api, purchase and planstep: ok.
• Merged tree (main plus this PR, previous version 133), with -tags integration (testcontainers Postgres):
  • TestMigration_PurchaseSuppressionsVariant: PASS.
  • A scratch up/down/up/down cycle on 50k pre-populated rows:
    • The table is not rewritten.
    • After down, the schema (columns, constraints and indexes) is identical to 133, and re-up is identical to the first up.
    • All rows are kept. A legacy row plus a variant row on one execution merges in down to 1 row with count 5.
  • The whole migrations package: ok.
  • TestSuppressionsScopedToPurchasedVariant_RealPostgres: ok.
  • config -run Suppress: ok.
• Fail-before: the scheduler real-Postgres test on base FAILS. other-az, regional, windows and dedicated each show 2 instead of 5.

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

[IAmJSD]

Local verification at f127e3f530f68c77a19608a06703c383340b7a20 merged onto current main (f9ae7663; merge-tree 062b0018; macOS):

• go build ./..., go vet ./... and go vet -tags integration ./... pass.
• go test -race -short -count=1 on the packages this PR changes passes: ./internal/config ./internal/database/postgres/migrations ./internal/scheduler.
• The full go test -race -short ./... suite passes on the tree with every PR in this merge train applied (tree 99ee50e2).
• Nothing was applied to any cloud.

[IAmJSD]

Marked ready and merged by IAmJSD's agent, at IAmJSD's request, after 2 independent reviews and local verification (see the review and verification comments above, all at f127e3f530f68c77a19608a06703c383340b7a20).