[IAmJSD]

Summary

This PR addresses the four 2026-09-02 audit findings bundled in the issue (A04-010 … A04-013). One part of A04-012 is deferred (normalizing the plan-services key delimiter, see below), so this is a partial fix. I re-checked each finding against current main (a14dc8f2), and all four still reproduced. git log --grep also turns up commit da731285, which mentions this issue number, but it is an old federation-test commit from before the renumbering and has nothing to do with these findings.

Refs #316

Per-finding status

Finding	Status on main a14dc8f2	This PR
A04-010 000059 down deletes Standard Users	Reproduces	Fixed. Only removes ...0005 when it really is Purchaser
A04-010 (same hazard) 000096 down	Partly: users_min_one_group already aborts the DO block atomically, so nothing is destroyed, but the error is a bare CHECK violation	Hardened. Refuses up front with an error naming the migration and the group
A04-010 (same hazard) 000024 down	Not a defect. 000024's down can only run after 000057's down, which drops users_min_one_group and restores users.role. An empty group_ids is then the legitimate pre-000024 state, so failing loud would block a valid rollback	No change
A04-011 000027 dedupes by ctid across partitions	Reproduces	Fixed. Joins on (tableoid, ctid)
A04-012 DerivePlanProviders fails open	Reproduces	Fixed. Accepts provider:service keys, identifies legacy-keyed entries by their Provider field, and errors instead of returning an empty set
A04-012 key-delimiter normalization migration	Reproduces: 000040 only rewrote colon keys, so API-written aws/savings-plans keys were never split	Deferred, see "Out of scope"
A04-013 MarkPurchaseRevoked leaves revocation_in_flight set	Reproduces	Fixed. SET list now clears it, and backfill migration 000146 fixes rows already revoked

Root cause and fix

A04-010: seed-group down migrations

000057 seeds Standard Users at 00000000-0000-5000-8000-000000000005. 000059 tried to seed Purchaser at the same UUID, and its ON CONFLICT DO NOTHING made that a no-op (#942). 000064 then put Purchaser at ...0007 and owns that row from then on. Its own down is a no-op. Even so, 000059's down removed ...0005 from every user and deleted the group. That destroyed Standard Users, or aborted on users_min_one_group if a user had no other group.

• 000059_seed_purchaser_group.down.sql: the membership strip and the delete now run only if the row at ...0005 is named Purchaser. If any user would be left with no group, the down raises a clear error instead. system_managed is still dropped. One thing stays as-is: 000059's admin backfill added ...0005 (Standard Users) to every admin, and those memberships can't be told apart from deliberate ones (documented in the file).
• 000096_seed_ri_exchanger_group.down.sql: the same up-front refusal for RI-Exchanger-only users, inside the existing DO block.

A04-011: 000027 dedupe

savings_snapshots is partitioned by month, and ctid is a position inside a single partition's heap. Matching on ctid alone meant a duplicate at (0,1) in one partition also deleted the row at (0,1) in every other partition. The CTE now selects tableoid and the DELETE matches on (tableoid, ctid). Duplicates of one (id, timestamp) always sit in the same partition, so the tableoid tie-break added to the ORDER BY never changes which row is kept. This edits an already-applied up migration. It only affects fresh databases and the forced re-run that the file's header describes.

A04-012: fail-open provider derivation

DerivePlanProviders cut keys on / and silently skipped anything else. That included the aws:savings-plans-<type> keys 000040 wrote, which findServiceKey in internal/api already accepts as valid. An empty result means "no restriction" to both guards (Handler.validatePlanAccountProviders and PostgresStore.validatePlanAccountProvidersTx), so a colon-keyed AWS plan accepted an Azure account. Now each entry's provider is resolved like this:

• Key prefix first: the text before the first / or : in the key.
• Then the entry's own Provider field, when the key has no prefix. This covers legacy aliases kept under their original key by fix(api): preserve legacy service aliases on plan update #1123's singleton-alias edit path (e.g. "original-key"), and plans keyed "ec2" with Provider: "aws". That is the same field the legacy edit path (mergeRequestedServices) and plan step resolution go by. Main skipped such entries, so a legacy-keyed plan accepted an account of any provider.
• An entry identified neither way is skipped. Skipping can only shrink the set, so the check never gets looser. TestPlanWriteBodiesAssociationCompatibility relies on this: it stores an unknown "future" services entry and expects account assignment to keep working.
• Fail closed: the function returns ([]string, error). If the plan has services but none of them identifies a provider, it returns an error. The handler maps that to a logged 500 (no plan UUID in the response), and the store returns it as-is.

Behavior change from main for mixed plans: a plan with aws/ec2 plus a legacy-keyed Azure entry now derives {aws, azure} instead of {aws}. That matches what the plan actually buys. A key prefix still wins over a conflicting Provider field, as before.

The first commit on this branch failed closed whenever no key had a provider prefix. That broke the supported legacy-key plans (TestLegacyServiceB1S1RoutedLedger, …RoutedRefusals, …Migration127Replay). The second commit (acd60dd4) adds the Provider-field fallback.

A04-013: revocation_in_flight never cleared

Migration 000072, PurchaseHistoryRecord.RevocationInFlight and the interface doc all say a successful MarkPurchaseRevoked resets the flag, but the UPDATE didn't. MarkPurchaseRevoked now sets revocation_in_flight = false in the same guarded UPDATE (WHERE revoked_at IS NULL). The new 000146_purchase_history_clear_revoked_in_flight clears the flag on rows that are already revoked. It leaves unrevoked flagged rows alone, because those are the finalize sweep's queue. Its down is a documented no-op.

Concurrency: concurrent MarkPurchaseRevoked calls on one row (the handler's retry loop plus the finalize sweep) serialize on the row lock. The losers re-check revoked_at IS NULL, match nothing and take the idempotent path. The integration test runs 8 concurrent marks on one in-flight row and checks that exactly one stamp lands and the flag ends cleared. A late FlipPurchaseRevocationInFlight from a stale concurrent request can still re-set the flag on a row that is already revoked. That was already possible before this PR (see "Out of scope").

Migration number

000146. Main ends at 000134. Open PRs use 000135–000143, our unopened fix/issue-47 takes 000144, and a sibling unopened branch (fix/issue-339) already has 000145. If those land after this one, 000146 has to be applied last or renumbered. The test uses previousMigrationVersion(t, 146).

Verification

All evidence below is local. "Integration" means real PostgreSQL in Docker, using the shared container (testhelpers.RequireMigratedDatabase). The handler tests are mock-store unit tests. Fail-before runs swapped in origin/main's production files (and an empty 000146 body), kept the new tests, and then restored the files.

export PATH=$HOME/.local/go/bin:$HOME/go/bin:$HOME/.local/bin:$PATH AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true
go build ./... ; go vet ./... ; go vet -tags integration ./...                      # all OK
go test -count=1 ./internal/config/ ./internal/api/ ./internal/database/postgres/migrations/ ./internal/mocks/   # one package at a time: all ok
golangci-lint run --allow-parallel-runners [--build-tags integration] ./internal/config/... ./internal/api/... ./internal/database/postgres/migrations/...   # 0 issues (both)
gocyclo -over 10 -ignore "_test\.go" <changed .go files>                          # none (validatePlanAccountProviders is at 10)
go test -tags integration -count=1 -timeout 20m -run 'TestMigration_SavingsSnapshotsPK_DedupeStaysInPartition|TestMigration_SeedPurchaserGroup_Down|TestMigration_SeedRIExchangerGroup_Down|TestMigration_000146_ClearRevokedInFlight|TestMigration_SeedRIExchangerGroup$' ./internal/database/postgres/migrations/
go test -tags integration -count=1 -timeout 20m -run 'TestMarkPurchaseRevoked_ClearsRevocationInFlight|TestSetPlanAccounts_ProviderCheckFailsClosed|TestPlanWriteBodiesAssociationCompatibility|TestIntegrationTestsShareOnePostgresContainer' ./internal/config/
go test -count=1 -run 'TestDerivePlanProviders' ./internal/config/ ; go test -count=1 -run 'TestSetPlanAccounts|TestValidatePlanAccountProviders|TestGetPlanForAccountProviderValidation' ./internal/api/

Test	Before (main code)	After
TestMigration_SeedPurchaserGroup_Down/Standard_Users_and_its_members_survive_the_rollback (integration)	FAIL: users_min_one_group violation	PASS
…/Standard_Users_is_not_silently_destroyed (integration)	FAIL: Standard Users deleted	PASS
…/a_Purchaser-only_user_stops_the_rollback_with_a_clear_error (integration)	FAIL: bare CHECK error	PASS
…/a_real_Purchaser_at_...0005_is_still_removed (integration, preservation)	PASS	PASS
TestMigration_SeedRIExchangerGroup_Down/an_RI-Exchanger-only_user_stops… (integration)	FAIL: bare CHECK error	PASS
TestMigration_SavingsSnapshotsPK_DedupeStaysInPartition (integration)	FAIL: other partition's row deleted	PASS
TestMigration_000146_ClearRevokedInFlight (integration)	FAIL with a no-op body: stale flag kept	PASS
TestMarkPurchaseRevoked_ClearsRevocationInFlight (integration, incl. 8-way concurrent marks)	FAIL: flag still true, row still in partial index	PASS
TestSetPlanAccounts_ProviderCheckFailsClosed (integration, store path: colon key, legacy key rejected and accepted, unidentifiable)	FAIL: Azure account accepted	PASS
TestSetPlanAccounts_ColonKeyedPlanStillChecksProvider, …_UnparseableServiceKeyFailsClosed, …_LegacyKeyedPlanUsesProviderField (unit, mock store)	FAIL: no error	PASS
TestDerivePlanProviders* (unit)	does not compile (signature change)	PASS
TestDerivePlanProviders_LegacyKeyUsesProviderField, TestSetPlanAccounts_LegacyKeyedPlanUsesProviderField, the legacy subtests of TestSetPlanAccounts_ProviderCheckFailsClosed	FAIL on the first commit's parser (aaeab564, the regression)	PASS

Regression check for the legacy-key plans, plus every test file that reaches SetPlanAccounts or createPlan (integration, run by name):

go test -tags integration -count=1 -timeout 60m -run 'TestLegacyServiceB1S1|PlanAccount|PlanProvider' ./internal/api/    # ok (39 top-level tests pass, incl. the three B1S1 tests)
go test -tags integration -count=1 -timeout 60m -run '^(<top-level tests of store_postgres_{delete_account,plan_writes,ramp_step}_integration_test.go>)$' ./internal/config/    # ok
go test -tags integration -count=1 -timeout 60m -run '^(<top-level tests of manual_plan_step, plan_step_cumulative, plan_step_retention, ramp_step_progress integration tests, fan_out_shape_test.go>)$' ./internal/purchase/    # ok
go test -tags integration -count=1 -timeout 60m -run '^(<top-level tests of handler_plans_{concurrency,legacy_version,managed_add_accounts,legacy_service_alias}_integration_test.go>)$' ./internal/api/    # ok
go test -count=1 ./internal/purchase/    # ok

I grepped internal/ for services fixtures whose keys have no provider prefix ("ec2", "original-key", "original-unusual-key", "one", "service"). Every one that reaches the provider check sets Provider and is covered by the runs above. The rest (mergeRequestedServices unit tests, plan_display_test.go, and the "aws|rds" key in recommendation_overrides_test.go, which is a different map type) never call DerivePlanProviders.

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

Branch	Tested head
fix/issue-337	ff3420c
fix/issue-169	7d75eb5
fix/issue-342	66b221f
fix/issue-310	5e11c21
fix/issue-326	5f0ea59
fix/issue-243	365e959
fix/issue-343	6c94036
fix/issue-312	d2848a8
fix/issue-316	acd60dd
fix/issue-339	75ea9b9
fix/issue-339-commitment-type	73bdb19
fix/issue-353	5d25fcd
fix/issue-313	3156bc4
fix/issue-357	53d9e58
fix/issue-368	0ba727d
fix/issue-419	61648c9
fix/issue-111	b04c372
fix/issue-686	0ad90b7
fix/issue-674	793095d
fix/issue-1055	4ec96f1
fix/issue-559	dfa9525
fix/issue-558	034eed4
fix/issue-874	1720539
fix/issue-666	1e120da
fix/issue-668	fb26345
fix/issue-568	ce04173
fix/issue-311	e54f250
fix/issue-177	0cbb88a
fix/issue-151	c21377c
fix/issue-166	41fb77b
fix/issue-27	2d67622
fix/issue-323	69288ae
fix/issue-59	36463e8
fix/issue-43	35e4069
fix/issue-158	6f76f8a
fix/issue-104	bb00491
fix/issue-55	971ef1b
fix/issue-361	89ac220
fix/issue-120	b458719
fix/issue-36	42bac6e
fix/issue-525	57c97dd
fix/issue-712	e5bcda0
fix/issue-72	6cc63ed

fix/issue-72 and fix/issue-312 conflict on one line in internal/api/handler_dashboard.go (both change the calculateCommitmentMetrics call). The tree used #72's side with #312's validated provider variable; whichever PR merges second takes that same resolution.

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:

• Passed at 936996ad: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks at 936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. tests/e2e is a separate module, so it was vetted under its own e2e tag. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package, one package at a time (-tags integration -count=1, 60-minute local timeout because the VM was shared):
  • At 936996ad, these pass: internal/server, internal/credentials, internal/scheduler, internal/database/postgres/migrations, internal/purchase and internal/api.
  • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
  • These were run on the first build of this tree (26795376, same branches at earlier heads): internal/analytics, internal/auth, internal/commitmentopts, internal/database and cmd/lambda/clear-rate-limit all pass. The later fixes don't touch those packages.
• What the combined run caught, each fixed on its own branch before this PR opened:
  • a compile break between fix/issue-104's test and fix/issue-326's httpToLambdaRequest signature change;
  • fix/issue-316 rejecting legacy-keyed plans;
  • fix/issue-120 versus main's global-filter test.
• Secrets: 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: 137 PASS, 0 FAIL.
• Nothing was applied to any cloud.

Out of scope / deferred

• Normalizing the plan-services key delimiter (A04-012, second half). The audit suggests one data migration that rewrites every key to a single delimiter, including splitting the API-written aws/savings-plans umbrella keys that 000040 never touched. I left it out for three reasons. Open fix(plans): migrate stored pre-#608 Azure/GCP service ids to canonical slugs #1140 (000136_canonical_legacy_plan_service_ids) rewrites purchase_plans.services too and deliberately keeps keys unchanged because fix(api): preserve legacy service aliases on plan update #1123's edit path relies on them. What to do when a plan has both aws/x and aws:x is an owner call. And the savings-plans identifier cleanup belongs to umbrella Normalize 'savings-plans' vs 'savingsplans' identifier across frontend + backend #7, which the maintainer asked to keep open. The fail-closed fix above removes the security-relevant part without it.
• FlipPurchaseRevocationInFlight has no revoked_at IS NULL guard, so a stale concurrent revoke request can re-flag a revoked row. Readers check revoked_at first, and that request's failed Return clears the flag again. Guarding it would change its not-found error semantics, so I left it unchanged.
• 000059's admin backfill put every admin in Standard Users (...0005). Rolling back can't tell those memberships from deliberate ones, so they stay.

No frontend change needed.

Maintainer: please mirror the issue's triage labels on this PR: impact/internal, triaged, priority/p3, severity/low, urgency/eventually, effort/m, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

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