[IAmJSD]

Root cause

Before #509, savePurchaseHistory hard-coded offering_class = 'convertible' for every AWS ec2 purchase_history row, whatever global_config.offering_class said. Rows bought while the setting was standard (from 425bc2f until #509 was deployed) are Standard RIs recorded as convertible, so isKnownNonStandardOfferingClass returns 400 on Sell and the History page hides the button. #509 fixed new writes but left the existing rows alone, and global_config keeps no history, so the database alone cannot tell which convertible rows are wrong.

Fix

Migration 000133_purchase_history_reverify_ec2_offering_class implements option (a) from the issue:

UPDATE purchase_history
SET offering_class = NULL
WHERE provider = 'aws'
  AND service IN ('ec2', 'compute')
  AND LOWER(offering_class) = 'convertible';

NULL is the column's existing "unknown" state. The History page already shows Sell for an empty class, and marketplaceList calls populateOfferingClass, which fetches the class from AWS (DescribeReservedInstances) and re-stamps the row. That path already exists and is covered by TestMarketplaceList_EmptyOfferingClassFetchedStandard / ...FetchedConvertible. No Go or frontend code changes.

• Both slugs are covered (legacy ec2 and canonical compute). This also clears post-fix(purchase): stamp the purchased EC2 offering class on history #509 rows where an idempotent re-drive stamped the class configured at retry time, as the issue's "Also" section describes.
• LOWER(...) matches the sell guard's case-insensitive comparison.
• standard stamps, other AWS services and other providers are left alone.
• Down migration: a deliberate no-op (SELECT 1). It can't tell which rows it cleared, and re-stamping NULL EC2 rows convertible would block Standard RIs again.
• Cost (accepted in option a): a genuinely Convertible, still-active RI now shows Sell. The first click returns 400 and re-stamps it convertible, so the button goes away after that. If AWS can't find the RI (expired or exchanged), the fetch fails with a 400 and the row stays NULL.

Concurrency: this is a single UPDATE inside the migration transaction. If the marketplace handler's StampOfferingClass writes standard on a row while the migration is running, the migration waits on that row lock. Under READ COMMITTED it then re-checks the WHERE clause against the committed row and skips it, so the AWS-verified standard survives. TestMigration133_ConcurrentStandardStampWins tests this on real Postgres: it holds the stamp transaction open, waits until pg_stat_activity shows the migration blocked on the lock, commits, then asserts standard. The opposite order (migration commits first, then the stamp) just leaves the stamp's value in place. A convertible stamp landing at the same moment can be cleared and re-fetched later, which is harmless.

Closes #524

Verification

All evidence below is local integration testing against a real Postgres 16 testcontainer, using fixture rows. No AWS calls were made.

AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null \
  go test -tags integration -count=1 -run 'TestMigration133' ./internal/database/postgres/migrations/ -v
# --- PASS: TestMigration133_ClearsConvertibleEC2Stamps
# --- PASS: TestMigration133_ConcurrentStandardStampWins

Fail-before: I replaced the up file with SELECT 1;. ClearsConvertibleEC2Stamps then fails on ri-ec2-convertible, ri-compute-convertible and ri-ec2-convertible-upper (each keeps convertible). ConcurrentStandardStampWins also fails, because no statement waits on the lock.

Mutation probes (each one makes the test fail):

Mutation	Failing case
drop LOWER()	ri-ec2-convertible-upper
drop provider = 'aws'	azure-compute-convertible
drop the service filter	ri-rds-convertible
service IN ('ec2') only	ri-compute-convertible
clear any non-NULL class	ri-ec2-standard, ri-compute-standard, and the race test

A down migration that re-stamped cleared rows would fail the rollback assertions.

Other checks:

• go test -tags integration -count=1 -p 1 ./internal/database/postgres/migrations/: ok (whole package, 135s)
• go test -count=1 ./internal/database/postgres/migrations/ ./internal/mocks/: ok (includes the transactional-statement guard and the repo-wide test guards)
• go build ./..., go vet ./internal/database/postgres/migrations/ and go vet -tags integration ...: clean
• golangci-lint v2.10.1 run --build-tags integration ./internal/database/postgres/migrations/: 0 issues
• gocyclo -over 10 -ignore "_test\.go": silent (no non-test Go changed)
• Pre-commit check-migration-conflicts: no duplicate numbers. 000133 is the next free number: main ends at 000130, and open PRs feat(exchange): record an audit trail for direct-API RI exchange executes #1092 and fix(analytics): cut savings_snapshots partitions at UTC month starts #1093 use 000131 and 000132.
• scripts/gosec-hook.sh on the new file: clean
• git-secrets --scan in an isolated temp repo: clean
• scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL
• Tests were run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because the default AWS profile on this machine is invalid. The migration doesn't use AWS.

Coverage gap: this hasn't been run against a real deployment's data or real AWS RIs. The issue's acceptance check ("no AWS ec2 row disagrees with AWS") holds for convertible because none remain after the migration. Each row only becomes AWS-verified when someone attempts Sell on it.

Out of scope

• Re-drive stamping at the source (the issue's "Also"). This needs an "already existed" signal on the go library's PurchaseResult. This migration clears any wrong convertible re-drive stamps that already exist, but new ones can still be written.
• The reverse direction: a row stamped standard that is actually Convertible (a re-drive under a standard setting). marketplaceList trusts a stored standard without asking AWS, and AWS would reject the listing. This is a separate issue, not part of this one.
• Option (b), a proactive per-row FetchOfferingClass job. Option (a) needs no per-account credential plumbing and no AWS traffic at deploy time.

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

🤖 Generated with Claude Code

[coderabbitai]

Note

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

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 147c00a1-be67-41fc-bc97-de96a0f30d0a

📥 Commits

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

📒 Files selected for processing: 3

• internal/database/postgres/migrations/000133_purchase_history_reverify_ec2_offering_class.down.sql
• internal/database/postgres/migrations/000133_purchase_history_reverify_ec2_offering_class.up.sql
• internal/database/postgres/migrations/000133_purchase_history_reverify_ec2_offering_class_test.go

+-------------------------------------------------+
| We don't ship debt; we refinance it into tests. |
+-------------------------------------------------+
  \
   \     /)/)
        ( '')
        /  |
      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]

Independent review (claude-cc-platform reviewer, cold brief): APPROVE 60bccb65e4ad2cc7c739614e3189336f6ddd8606

No blocking findings. The PR is one commit: migration 000133 (up and down) plus an integration test. It implements option (a) from #524.

Rows touched: AWS EC2 only (provider='aws', service IN ('ec2','compute'), case-insensitive 'convertible'). NULL rows are unaffected. Up is idempotent. Down is a no-op SELECT 1.

Consumers:

• history.ts:762-763 shows Sell for 'standard' or empty.
• handler_marketplace.go:210 gates on isKnownNonStandardOfferingClass, which is false for empty.
• populateOfferingClass (:335) re-stamps the row from AWS.

So a genuinely Convertible, still-active RI shows Sell once, and its first click returns 400 and re-stamps it. The PR discloses this.

Attacked:

• go vet -tags integration on the migrations package is clean.
• go test -tags integration -run TestMigration133 passes against Docker Postgres, including the concurrent-stamp race test.
• Mutation probes, each failing the test:
  • provider='aws' filter dropped;
  • service narrowed to 'ec2';
  • LOWER() removed;
  • down migration made to re-stamp 'convertible'.

Base drift: clean merge onto current main; the three files are new.

Nits:

• Migration ordering: main ends at 000130, and 000131/000132 and 000134-000137 are claimed by open PRs feat(exchange): record an audit trail for direct-API RI exchange executes #1092, fix(analytics): cut savings_snapshots partitions at UTC month starts #1093, fix(purchase): scope purchase suppressions to the AZ, platform and tenancy bought #1110, fix(auth): bind group account scope entries to account IDs #1116, fix(plans): migrate stored pre-#608 Azure/GCP service ids to canonical slugs #1140 and fix(db): let ri_exchange_history.account_id hold Azure and GCP identifiers #1170. golang-migrate only applies versions above the current one, so if 000133 lands before 000131/000132, those two are silently skipped on already-migrated databases. These must merge in numeric order.
• The single UPDATE is unbatched. purchase_history is small, and the READ COMMITTED analysis holds.
• The write path (execution.go:1017-1019) can still stamp the configured class on re-drives. The PR defers this to a separate issue.

[cristim]

Independent review (claude-fleet): APPROVE 60bccb6

The migration is correct, idempotent and tightly scoped, and every probe was caught by a test. The one real risk is merge order, which nothing in the repo or CI checks.

Findings (none blocking):

• [non-blocking, process] Merge order of 000131/000132 against 000133 is not enforced.
  • Failure scenario: if fix(history): clear stale convertible stamps on AWS EC2 RI history rows #1105 merges and deploys before feat(exchange): record an audit trail for direct-API RI exchange executes #1092 (000131) and fix(analytics): cut savings_snapshots partitions at UTC month starts #1093 (000132), any database already at 133 never applies 131 or 132. m.Up() (internal/database/postgres/migrations/migrate.go:62) only applies versions above the current one.
  • The only guard is the pre-commit duplicate-number hook (.pre-commit-config.yaml:203-205), which catches duplicates, not order. Gaps are intentional in this repo (helpers_test.go:52-57).
  • Fix: merge in numeric order, or renumber at merge time.
• [non-blocking, disclosed] 000133 down.sql:6 is a no-op. A down-then-up cycle runs up again, which clears convertible stamps that populateOfferingClass already re-verified from AWS (handler_marketplace.go:335-347). Each affected RI then costs one extra refused Sell (400) to re-stamp. No data is destroyed.
• [non-blocking, disclosed] up.sql:24-28 also clears genuinely Convertible rows written after fix(purchase): stamp the purchased EC2 offering class on history #509 (the default setting is convertible, execution.go:1017-1019). This is the accepted option (a), and the PR body states the cost.

Migration order: main ends at 000130. Open PRs claim 000131 (#1092), 000132 (#1093), 000133 (#1105, unique), 000134 (#1110), 000135 (#1116), 000136 (#1140) and 000137 (#1170), with no duplicates. A lower number must deploy before any higher one, or it is silently skipped on databases that are already migrated. Nothing enforces this: CI's migrate up (ci.yml:435) runs on a fresh database, where order never matters. This PR depends on no other open PR's data.

Attacked:

• Refuted:
  • Spelling drift: the write-path slugs are exact lowercase (execution.go:1262-1279, :1018), and the WHERE clause uses the same literals.
  • Other readers of convertible: only history.ts:762-763 and isKnownNonStandardOfferingClass (handler_marketplace.go:531) read it, and both treat empty as "unknown, allow". The exchange path (handler_ri_exchange.go:1627) never reads purchase_history.offering_class.
  • Lock scope: a single UPDATE with row locks only, with no ALTER, rewrite or ACCESS EXCLUSIVE lock. The table is small.
  • Cross-tenant scope: intended, since the defect is global. Other providers and services are untouched, which is tested.
  • Earlier-migration dependency: the column comes from 000087 (nullable TEXT), and LOWER(NULL) skips NULL rows.
• Also attacked: the WHERE clause against NULL, mixed case, standard rows, and other providers and services; the concurrent standard stamp race under READ COMMITTED; and whether down is a true inverse.
• CI coverage: the test is //go:build integration, so ci.yml:466 picks it up. It runs the real migration files through MigrateToVersion against a Postgres testcontainer.
• Mutation probes, each restored and confirmed identical with cmp:
  • P1, the service filter dropped: FAILS on ri-rds-convertible.
  • P2, the LOWER(offering_class)='convertible' guard dropped: FAILS on the standard rows and the race test.
  • P3, down re-stamps NULL rows: FAILS the rollback assertions.
  • P4, provider='aws' dropped: FAILS on azure-compute-convertible.

Verified locally, on a git archive export of HEAD with Docker and no cloud credentials:

• go build ./..., go vet and go vet -tags integration on the migrations package: clean.
• go test -count=1 -tags integration -run TestMigration133 ./internal/database/postgres/migrations/: both tests PASS.
• Against the base export, without the migration files: both FAIL.

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

[cristim]

Local verification (macOS, exact head 60bccb65e4ad2cc7c739614e3189336f6ddd8606, detached worktree):

• go build ./..., go test -race -short -count=1 ./... and make lint pass.
• go vet -tags integration ./internal/database/... is clean.
• go test -race -tags integration -run TestMigration133 against Docker Postgres: 2 PASS, 0 SKIP, 0 FAIL.

Gate status: two independent reviews approve this SHA. Merge waits on @IAmJSD marking the PR ready (see #974), and on migration order: 000131 (#1092) and 000132 (#1093) must merge first, because golang-migrate skips lower versions on an already-migrated database.

[cristim]

Local verification at 60bccb65e4ad2cc7c739614e3189336f6ddd8606 merged onto current main (4e2ed58a; merge-tree 56b9e185; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• go test -tags integration ./internal/database/... passes on real Postgres (testcontainers), with every migration now on main applied first.
• Nothing was applied to any cloud.

[cristim]

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