[IAmJSD]

Root cause

Recommendations carry the provider's common.ServiceType slug (compute, relational-db, nosql, cache, search). The scheduler (config.ResolveAccountConfigsForRecs) and the purchase modal look up service_configs and account_service_overrides by that exact (provider, service) pair. Before #1354, the Settings page saved Azure and GCP rows under per-product ids (azure/vm, azure/sql, azure/cosmosdb, azure/redis, gcp/sql, gcp/memorystore). No recommendation carries those ids, so those defaults and overrides never applied.

#1354 fixed what the page writes. It re-keys old rows only in the browser: an old service config moves to the new id on the next Save, and an old override only shows a hint. The rows stored in the database stayed under the old ids. Until an admin re-saves Settings, they are still dead. Old overrides stay dead until someone re-creates them.

Fix

Migration 000151_canonical_legacy_settings_service_ids rewrites the stored rows. It uses the mapping from LEGACY_SETTINGS_SERVICES in frontend/src/lib/settings-service-ids.ts. There is no backend canonicalizeServiceConfigs; the only one is the frontend function in that file. The precedence is the same as there:

• A canonical twin exists (same provider and canonical service, and for overrides the same account): the twin wins and the legacy row is deleted. The legacy row never applied anyway.
• Otherwise: the legacy row is re-keyed in place. Its id, settings and created_at stay the same, and the table's update trigger bumps updated_at.
• Every other row is left alone. That includes gcp/cloudsql, which is a plan id that 000136 already handled and that Settings never wrote, and legacy names under another provider (for example aws/vm).

The mapping never sends two legacy ids of one provider to the same canonical id, so two re-keys can't collide.

The header has a read-only detection query, as 000136 does. It lists every row the migration will rewrite and whether a twin will win.

Down migration: a no-op, documented in its header.

• A re-keyed row looks the same as a row saved under the canonical id, so the down migration can't tell which rows to move back. Moving them back would also bring the bug back.
• A service_configs legacy row that lost to its twin is gone for good. That table has no audit trail.
• An override that lost to its twin can be recreated by hand from its audit event (next section).

Audit trail (#1493 / 000147): rows are written

I chose to write config_audit_events rows for every override the migration changes. Deleting a legacy twin destroys user-entered settings, and the trail is the only place they would survive.

• Superseded legacy override: one account_service_override.delete event, with before set to the deleted row's settings.
• Re-keyed override: a delete event under the legacy id and an upsert (create) event under the canonical id, both carrying the same settings.
• All events use actor migration 000151 and details {"migration":"000151","reason":…,"legacy_service":…,"canonical_service":…}. They use the existing event types, so no CHECK constraint changes.
• before/after have the same shape as overrideSnapshotJSON. A test saves the same settings through SaveAccountServiceOverrideAuditedTx and checks that the two after values are equal as jsonb.
• service_configs rewrites record nothing, because feat(audit): record config_audit_events for account service override writes (single and import) #1493's trail covers only overrides.

Concurrency

RunMigrations runs on demand while other instances keep serving requests. The file first runs LOCK TABLE service_configs, account_service_overrides IN EXCLUSIVE MODE. Plain reads still work, but:

• An open transaction that has written a legacy row is waited for, and the row it commits is rewritten. Without the lock, the migration's statements would not see that uncommitted row, and it would end up committed under the old id after the migration.
• Writers that arrive during the migration queue behind it. That includes the SELECT … FOR UPDATE reads in UpdateServiceConfigAtomic and SaveAccountServiceOverrideAuditedTx. They then read the re-keyed row, so the merge base and the audit event's before are the row they actually replace.

The migration takes no advisory locks, and no application transaction writes both tables, so there is no lock-order cycle. Both tables are small (one row per service, and per account for overrides).

Closes #613

Verification

Evidence here is local integration tests on real Postgres 16 (testcontainers), on this branch at the PR head. It is not a deployed environment.

export PATH=$HOME/.local/go/bin:$HOME/go/bin:$HOME/.local/bin:$PATH
export AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true
go build ./... && go vet ./... && go vet -tags integration ./...          # OK
go test -count=1 ./internal/database/postgres/migrations/               # ok
go test ./internal/mocks/ -count=1                                       # ok
golangci-lint run --allow-parallel-runners --build-tags integration ./internal/database/postgres/migrations/   # 0 issues
go test -tags integration -count=1 -timeout 20m -v \
  -run 'TestMigration_CanonicalLegacySettingsServiceIDs|TestMigrationsAreTransactional' \
  ./internal/database/postgres/migrations/
# --- PASS: TestMigrationsAreTransactional
# --- PASS: TestMigration_CanonicalLegacySettingsServiceIDs_ConcurrentWriters
# --- PASS: TestMigration_CanonicalLegacySettingsServiceIDs (+4 subtests)

What the tests cover:

• TestMigration_CanonicalLegacySettingsServiceIDs:
  • All six mappings in service_configs.
  • A twin conflict in each table.
  • Twin detection per account: account B's azure/redis is re-keyed even though account A has azure/cache.
  • Untouched rows (canonical ids, gcp/cloudsql, aws/vm) keep updated_at.
  • Re-keyed rows keep their id.
  • The exact audit event sequence and payloads.
  • The snapshot-shape check against the store.
  • Down + up changes nothing (rows, updated_at, audit events), and the helper functions are dropped.
• TestMigration_CanonicalLegacySettingsServiceIDs_ConcurrentWriters:
  • Uncommitted legacy writes in both tables make the migration wait.
  • A queued UpdateServiceConfigAtomic merges onto the re-keyed gcp/sql → gcp/relational-db row.
  • A queued SaveAccountServiceOverrideAuditedTx replaces the re-keyed override, and its audit before is the re-keyed row's settings.

Fail-before and mutation probes (each one reverted after the run):

• Migration files removed (state before the fix): TestMigration_CanonicalLegacySettingsServiceIDs FAILS with no migration found for version 151.
• LOCK TABLE line removed: …_ConcurrentWriters FAILS with "the migration must wait for the open service_configs writer".
• service_configs twin DELETE removed: the main test FAILS with a unique violation during the migration.
• Override twin check made global instead of per account: the main test FAILS (B's override is dropped instead of re-keyed, and the audit events don't match).

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 400d256b = origin/main 7b86e386 plus these heads, merged in this order:

Branch	Tested head
fix/issue-243-rest	007c163
fix/issue-1206	312072f
fix/issue-111-email	6d422e2
fix/issue-111-fail-closed	3817e29
fix/issue-712-age-cap	a91120b
fix/issue-712-fargate	aeaf0d4
fix/issue-613-legacy-ids	350e837
fix/issue-120-guards	6f38ebc
fix/issue-120-shared-predicate	cb25735
fix/issue-151-account-update-keep	d20cf01
fix/issue-713-upcoming-filter	e73fdfb
fix/issue-263-bastion-opts	7861f32
fix/issue-43-fixture-ids	8cee203
fix/issue-177-makefile-walk	365a9d6
fix/issue-27-seller-not-registered	d845f66
fix/issue-313-user-columns	6d759dd
fix/issue-353-mock-panics	aac2edf
fix/issue-8-config-stamps	b548ae5
fix/issue-30-monthly-totals	f637921

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran on f7c48020, which differs from this tree only by test-only commits on fix/issue-30, fix/issue-713 and fix/issue-263 that replace placeholder account-number literals with named constants:

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package that has them, one package at a time (-tags integration -count=1): internal/analytics, internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase, internal/scheduler and internal/server all pass. Migrations 000149, 000150 and 000151 apply in order on this tree.
• Frontend: the changed jest files (approval-details, settings-accounts, api) pass (221 tests).
• Secrets: the first run flagged 9 new fixture lines (placeholder account numbers next to an account key); fixed with named constants. Re-run on this tree: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.
• Terraform .tftest.hcl files were not run locally (CI runs them). Nothing was applied to any cloud.

Out of scope

• The frontend re-key and hint fallback (canonicalizeServiceConfigs, the legacy-override note) stays. Removing it is the UI owner's call (see below).
• The backend still accepts any service id on PUT /config/service/{provider}/{service} and on override writes. An old browser tab running the pre-fix(frontend): save Azure/GCP settings under recommendation ids #1354 bundle could still write a new legacy row after this migration. The frontend fallback still handles such a row. Rejecting or aliasing legacy ids at the API would be a separate backend change.

Frontend follow-up for the UI owner

No UI change is needed for this PR to work. Once 000151 has run everywhere, these become dead code for migrated databases, and the UI owner can remove them:

• The browser-side re-key in canonicalizeServiceConfigs.
• The legacy-override hint in frontend/src/settings.ts.

Keep them while old bundles might still write legacy ids (see Out of scope).

Labels

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

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 7b86e38
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: a56868bd-fc69-46cf-8d49-ec1b67c44a57

Commits

Reviewing files that changed from the base of the PR and between 7b86e38 and 9848196.

Files ignored due to path filters: 1

• go.sum is excluded by !**/*.sum

Files selected for processing: 5

• go.mod
• internal/database/postgres/migrations/000151_canonical_legacy_settings_service_ids.down.sql
• internal/database/postgres/migrations/000151_canonical_legacy_settings_service_ids.up.sql
• internal/database/postgres/migrations/000151_canonical_legacy_settings_service_ids_race_test.go
• internal/database/postgres/migrations/000151_canonical_legacy_settings_service_ids_test.go

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 350e8371f18a73e5c79cad59c8543ef75c33799a

Blocking:

• none

Non-blocking:

• Merge order (owner attention): this is 000151 while main is at 000148. 000149 is fix(config/db): settled-only expiry cleanup, RI-exchange limit CHECKs, capped list reads (#120 slice) #1552 and 000150 is fix(config/db): share one account predicate and key the RI utilization cache by account #1553. golang-migrate's Up only applies versions above the recorded one. If this PR merges and deploys before those two, a database that has recorded 151 will silently skip 149 and 150 when they land later. So merge fix(config/db): settled-only expiry cleanup, RI-exchange limit CHECKs, capped list reads (#120 slice) #1552, then fix(config/db): share one account predicate and key the RI utilization cache by account #1553, then this one, or renumber whichever lands out of order.
• 000151...up.sql LOCK: an app transaction that already holds a write or FOR UPDATE lock on account_service_overrides and then writes service_configs would deadlock against the ordered LOCK TABLE. I grepped and found no such transaction today. Only store_postgres.go touches both tables, in separate functions, and the override import (handler_override_import.go) writes overrides only. Postgres would abort one side and the migration would fail cleanly in its own transaction. Noting it for future writers.
• As the PR body says, a stale pre-fix(frontend): save Azure/GCP settings under recommendation ids #1354 browser tab can still write a legacy id after this runs. The frontend fallback covers it.

Attacked:

• Checked the mapping against LEGACY_SETTINGS_SERVICES and canonicalizeServiceConfigs (same six pairs; the twin wins). The per-account twin check uses l.account_id. override_snapshot_000151 matches overrideAuditSnapshot's JSON keys and its NULL/[] handling. The events meet config_audit_events' CHECKs (delete has before set and after NULL; upsert has after). uuid_generate_v4 comes from 000001. Issue fix(frontend): settings service overrides still use the old Azure/GCP service ids #613's slug-list test already exists in settings-service-ids.test.ts, so Closes is justified.
• Ran go build ./...; go vet with and without -tags integration; go test -race -short on migrations; go test -race -tags integration -run 'TestMigration_CanonicalLegacySettingsServiceIDs|TestMigrationsAreTransactional' (all PASS); -run 'DownAll|Idempot' (PASS, covers the no-op down); golangci-lint --build-tags integration (0 issues).
• Fail-before: with the up file reduced to SELECT 1;, TestMigration_CanonicalLegacySettingsServiceIDs fails (3 subtests). With only the LOCK TABLE line deleted, ..._ConcurrentWriters fails with "the migration must wait for the open service_configs writer". Both restored.

Body-stale:

• none

_{Reviewer: rG2. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 984819639c96a9ea1cb6e01c14255148e68573dc

Blocking:

• none

Non-blocking:

• A browser tab still running a pre-fix(frontend): save Azure/GCP settings under recommendation ids #1354 bundle can write a legacy-id row after this migration runs, and that row would be dead again. The window is small. canonicalizeServiceConfigs fixes service configs on the next Save, but a late legacy override would need to be re-created by hand.
• Merge order: 000151 must land after fix(config/db): settled-only expiry cleanup, RI-exchange limit CHECKs, capped list reads (#120 slice) #1552 (000149) and fix(config/db): share one account predicate and key the RI utilization cache by account #1553 (000150). golang-migrate never applies a file numbered below the database's current version.

Attacked:

• Mapping: matches LEGACY_SETTINGS_SERVICES exactly. No two legacy ids of one provider map to the same canonical id. No backend code still uses vm, cosmosdb, memorystore or redis literals.
• Locking: LOCK service_configs, account_service_overrides IN EXCLUSIVE MODE. No app transaction touches both tables. UpdateServiceConfigAtomic only touches service_configs, and the override save only touches overrides plus audit, so there is no lock-order cycle. Plain reads aren't blocked.
• Down migration is a no-op, which is defensible and documented.
• Closes #613 holds: fix(frontend): save Azure/GCP settings under recommendation ids #1354 covered the UI ids and the slug test, and this PR migrates the stored rows.
• Mutation probes:
  • Removing the LOCK makes TestMigration_CanonicalLegacySettingsServiceIDs_ConcurrentWriters FAIL.
  • Disabling the service_configs twin-wins check makes TestMigration_CanonicalLegacySettingsServiceIDs FAIL.
  • Both reverted.
• Ran:
  • go build ./...
  • go vet ./..., with and without -tags integration
  • go test -race -short -count=1 on migrations: ok
  • go test -race -tags integration -run 'TestMigration_CanonicalLegacySettingsServiceIDs|TestMigrationsAreTransactional': all PASS
  • golangci-lint --build-tags integration on migrations: 0 issues
  • On a local tree of main + fix(config/db): settled-only expiry cleanup, RI-exchange limit CHECKs, capped list reads (#120 slice) #1552 + fix(config/db): share one account predicate and key the RI utilization cache by account #1553 + this PR, the 000149, 000150 and 000151 tests plus TestMigrations_DownAllRoundTrip and TestMigrations_DownToOneRoundTrip all PASS in order.

Body-stale: none (the head also carries the golang.org/x/net bump, not mentioned).

_{Reviewer: rH2. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 984819639c96a9ea1cb6e01c14255148e68573dc merged onto main (bbf0a839), together with #1552, #1553, #1551, #1246, #1548, #1549, #1554, #1560 in that order (combined tree 7f9817d4, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on 98481963, green CI, and the combined-tree verification above. Squash-merged at exactly 984819639c96a9ea1cb6e01c14255148e68573dc.