[IAmJSD]

Root cause

Every purchase_history row is written by one Go function: PostgresStore.SavePurchaseHistory, called only from purchase.Manager.savePurchaseHistory. That function stored exec.CloudAccountID as given. Only the multi-account fan-out sets it. On the single-account path it is nil:

• direct execute: PlanID == "", and the account is taken from the recs;
• legacy plan path: a plan with no plan accounts;
• ambient path: the host's STS identity, no account.

On all three, resolveSingleAccountProvider resolves the account and stamps its ExternalID on AccountID, but nothing put the UUID on cloud_account_id. So new NULL rows kept appearing after the 000139 backfill (#1201).

Fix

SavePurchaseHistory now fills a missing cloud_account_id inside the INSERT itself. It uses COALESCE($17::uuid, (SELECT id FROM cloud_accounts ... FOR KEY SHARE)), which:

• covers every writer, including future ones, at the single point all writes go through;
• uses the backfill's rules exactly:
  • join on (provider, external_id), the cloud_accounts UNIQUE key;
  • provider compared lower-cased (legacy rows say AWS);
  • account id compared exactly, as the readers do;
  • an empty account id never matches;
• keeps an explicit CloudAccountID from the caller, as before;
• is not evaluated when the NOT EXISTS dedupe skips the insert, so an existing row is never rewritten.

Concurrency. The lookup locks the account FOR KEY SHARE.

• Without the lock: an account delete that commits between the lookup and the FK check aborts the insert with an FK violation. The purchase has already happened, so the history row is lost (audit gap).
• With the lock: the insert waits for an in-flight delete, finds no account, and stores NULL, the same value ON DELETE SET NULL would leave.
• A delete that starts later waits for the insert and then clears the column itself.
• Both orders are tested on real Postgres.

"Fail loudly". I did not make an unresolvable account refuse the row.

• When a row has no account, the commitment has already been bought. Refusing the row would recreate the fix(api): skip plan disable on planned purchase delete when the plan spans out-of-scope accounts #621 audit gap.
• Ambient-only deployments with no registered accounts are supported (SingleCloudAccountIDFromRecs returns nil for them). Refusing would hide every purchase they make.
• Instead, such a row is stored with NULL and a Warnf names the provider and account id. Readers still match it by the external account_id.
• A DB error in the lookup fails the statement, and so the save, as before.

If you'd rather refuse, say so: it's a one-line change. The NOT NULL constraint the issue mentions should wait until it's clear that ambient-only deployments can't produce unlinked rows.

SavePurchaseHistory moved out of the 4.5k-line store_postgres.go into internal/config/store_postgres_history_save.go. The SQL now uses QueryRow ... RETURNING cloud_account_id IS NOT NULL so it knows when to warn. pgx.ErrNoRows keeps the old "already recorded" path.

Behavior change worth a look

New host-account rows written by the ambient path are now linked to the registered account with the same external id. That makes them visible to account-scoped permission checks:

• an account-constrained sell grant now matches such a row;
• before, the row was unattributed and failed closed.

The 000139 backfill does the same for existing rows, and the marketplace path already resolved such rows to that account for signing (#560). TestMarketplacePermissionConstraintsHTTP:

• the missing-account / own-missing-account cases now clear the resolved id after the save, so they still test a legacy unlinked row;
• the new resolved-account case pins the new behavior.

Verification

All on macOS against real Postgres 16 via testcontainers. I ran the tests with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid. No cloud APIs were called.

Fail-before (base c8bdd460 plus only the new test file):

go test -tags integration -count=1 -run 'TestSavePurchaseHistory_(StampsCloudAccountID|ConcurrentAccountDelete|ConcurrentSavesAndDelete)' ./internal/config/
--- FAIL: TestSavePurchaseHistory_StampsCloudAccountID   (5 subtests: expected <uuid>, actual "")
--- FAIL: TestSavePurchaseHistory_ConcurrentAccountDelete (the save never resolves the account, so it never waits on the delete)

Pass-after:

• go test -tags integration -count=1 ./internal/config/: ok
• go test -tags integration -count=1 ./internal/api/: ok (273s, whole package)
• go test -tags integration -count=1 -run '^TestMigration095_' ./internal/database/postgres/migrations/: ok
• go test -count=1 on ./internal/config/ ./internal/mocks/ ./internal/purchase/ ./internal/api/: ok
• go test -short -race -count=1 ./internal/config/: ok

Mutation probes (scratch edits of the SQL, each restored; the final file is byte-identical). All were caught:

Mutation	Caught by
drop FOR KEY SHARE	ConcurrentAccountDelete, ConcurrentSavesAndDelete (FK violation)
drop LOWER()	legacy_upper-case_provider
drop the provider join	ambient_aws_host_account, azure_row_with_the_shared_number, legacy_upper-case_provider
drop $1 <> ''	empty_account_id
lookup wins over the caller's id	an_explicit_cloud_account_id_is_kept, TestPGXMock_..._ResolvesAccountAndKeepsUnmatchedRow
no lookup ($17 only)	the direct-execute and ambient subtests
case-insensitive external id	subscription_differing_only_in_case

Static checks:

• go build ./..., go vet (with and without -tags integration) and gofmt: clean.
• golangci-lint v2.10.1 run --build-tags integration ./internal/config/... ./internal/api/...: 0 issues.
• gocyclo -over 10 on the changed non-test files: silent.
• gosec hook (scripts/gosec-hook.sh, v2.28.0): exit 0.
• git-secrets (--register-aws as in CI, isolated HOME, fresh repo with the changed files git added):
  • clean;
  • a planted key and account id in the same repo were flagged;
  • scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.

Coverage gaps:

• This is store-level evidence on real Postgres, with rows shaped like each write path. No end-to-end purchase was run against a real cloud account.
• The account-delete race is tested deterministically: a held delete transaction, with the save checked to be waiting on its lock in pg_stat_activity. There is also a 20-way concurrent smoke test.

Out of scope / notes

• Existing race on the fan-out path. The fan-out path passes an explicit account.ID. If that account is deleted mid-purchase, the insert can still fail on the FK, which is an audit gap. I kept explicit ids FK-enforced rather than silently nulling a bogus id. Worth a separate issue if you want it closed too.
• Accounts registered mid-save. An account registered concurrently with a save is not visible to that save's lookup, so the row stays NULL. Rerunning the 000139 backfill would link it.
• NOT NULL constraint. Not added here; see "Fail loudly" above.
• No migration, so no migration-number change.

Overlap with open PRs

• chore(db): backfill purchase_history.cloud_account_id from (provider, external_id) #1201 (000139 backfill): complementary, no file overlap.
• feat(api): scope-aware SQL totals for the History summary cards #1200 (History summary cards): likely semantic conflict, though not a textual one. Its integration fixtures seed "legacy" rows through SavePurchaseHistory with CloudAccountID: nil and a registered account_id. After this PR, those rows get linked automatically. If the fixtures need truly unlinked rows, they need the same UPDATE ... SET cloud_account_id = NULL used here, or a raw INSERT.
• fix(config): use the earliest row when purchase_history duplicates a purchase_id #1103, fix(api): four handler bugs found while documenting the API (#1102) #1141: their tests call SavePurchaseHistory with no matching registered account (or with an explicit id), so no behavior change is expected.
• No hunks overlap the moved function or the edited pgxmock test in sec(purchase): pin the approval token in the token approve/cancel CAS; compare revoke token first #1242, feat(plans): settle a failed-with-submission row so a managed plan can be deleted #1241, chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217, fix(purchase): fail a plan step's root row atomically with its reason and suppression release #1176, fix(api): answer a missing RI exchange like a wrong token on approve and reject #1175, fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172, fix(api): apply the RI exchange history limit after the account filter #1169 or fix(config): lock a deleted account's plans before disabling them #1087.
• fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending #1237, fix(purchase): guard the remaining post-CAS full-row saves on status #1111: they touch handler_purchases.go and purchase/*, not this code, so no conflict.

Frontend follow-up for the UI owner

None needed. History already matches rows by either id.

Closes #1235

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

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

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 2 (claude-fleet, cold): APPROVE bc2581c

Findings (none blocking):

• [non-blocking, pre-existing, known-issues] internal/config/store_postgres_comprehensive_test.go:349 - mockablePostgresStore.SavePurchaseHistory is a stale 16-column VALUES copy of the production writer.
  • The three TestSavePurchaseHistory_* mocks (:1302, :1350, :1392) test that copy, never the real SQL.
  • So they give false coverage: no cloud_account_id and no dedupe.
• [non-blocking] Merged-tree overlap with feat(api): scope-aware SQL totals for the History summary cards #1200, which is now on main.
  • internal/api/handler_history_summary_integration_test.go:70-83 seeds "legacy" rows through SavePurchaseHistory, with CloudAccountID nil and a registered aws external id. With this PR, those rows now get linked.
  • The tests still pass, but the api summary tests no longer cover an unlinked row, so the "legacy account id" case at :338 no longer tests what its name says.
  • The config summary fixture still has one unlinked row (provider "", store_postgres_history_summary_integration_test.go:82).
• [non-blocking] store_postgres_history_save.go:104 - The Warnf for an unlinked row has no test (M7 survives). In an ambient-only deployment it fires on every purchase.
• [design note, refuted as a bug] execution.go:452-468 already holds the resolved account and passes on only ExternalID.
  • The SQL lookup gets the same UUID, because of UNIQUE(provider, external_id) plus the lower-case CHECK.
  • It differs only if rec.Provider differs from account.Provider, and then the row gets NULL, not the wrong account.

Attacked:

1. Writers:
   • The only production INSERT, in head and in the merged tree, is store_postgres_history_save.go:35. Its only caller is purchase/execution.go:1024, reached via :738.
   • Every other INSERT is a test seed or a migration test. The CopyFrom in analytics writes savings_snapshots.
   • ON CONFLICT DO NOTHING returns no row, ErrNoRows is handled, and the "already recorded" path still works.
2. Where the ID comes from:
   • The lookup keys only on the row's own account_id ($1) and provider ($4), with no session input.
   • It cannot resolve across providers (the shared-number aws/azure subtests, and M8).
   • cloud_accounts has a global UNIQUE(provider, external_id) (000011:38), so more than one match is impossible.
   • No match: the row is stored with NULL (4 subtests).
   • Lookup error: the save fails, the caller records an audit gap, and the purchase stays completed (execution.go:738-745, pgxmock DBError).
3. chore(db): backfill purchase_history.cloud_account_id from (provider, external_id) #1201's backfill:
   • Its predicate is the same as this PR's: account_id <> '', an exact external_id match and LOWER(ph.provider) = ca.provider. Both take FOR KEY SHARE, which is compatible.
   • A scratch probe held the 000139 UPDATE uncommitted while running a save. The save did not block, linked its own row and skipped the duplicate key.
   • Readers (appendAccountPredicate, store_postgres.go:2537) still match NULL rows by account_id.
4. Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
   • M1, LOWER dropped: caught.
   • M2, FOR KEY SHARE dropped: caught by both concurrency tests.
   • M3, the lookup wins over $17: caught.
   • M4, a case-insensitive external_id: caught.
   • M5, the empty-id guard dropped: caught.
   • M6, ErrNoRows treated as an error: caught.
   • M7, the Warnf condition inverted: survives. This is the gap above.
   • M8, any provider matched: caught.

Verified locally, with GOTOOLCHAIN=go1.26.10 and no cloud credentials, under the heavy lock, on testcontainers Postgres:

• Head:
  • go build ./...: ok.
  • go vet, with and without -tags integration: ok. gofmt -l: clean.
  • Unit tests on config, mocks, purchase and api: pass.
  • Integration -run 'TestSavePurchaseHistory_|TestPGXMock_SavePurchaseHistory|History' ./internal/config/: pass.
• Fail-before, on base plus the new test file:
  • StampsCloudAccountID FAILS 5 subtests, and ConcurrentAccountDelete FAILS.
  • ConcurrentSavesAndDelete passes on base, because without the lookup there is no FK to break.
• Merged tree: merge-tree with origin/main 6af022f is clean.
  • Build and vet: ok.
  • The full -tags integration ./internal/config/: passes.
  • The internal/api integration tests for HistorySummary, MarketplacePermissionConstraintsHTTP and NullablePurchaseCostPostgres: 97 PASS, 0 FAIL.
• No migration.

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

[cristim]

VERDICT: APPROVE bc2581c

Findings (none blocking):

• non-blocking internal/config/store_postgres_history_save.go:103-111 - the issue asked to "fail loudly" when no account resolves; the PR stores NULL plus a Warnf instead. I agree: refusing the row recreates the fix(api): skip plan disable on planned purchase delete when the plan spans out-of-scope accounts #621 audit gap and breaks ambient-only deployments. This is an owner decision, and the PR asks for it.
• non-blocking internal/config/store_postgres_history_save.go:41-48 - the explicit-id path (fan-out) still FK-aborts if the account is deleted mid-purchase. The PR discloses this as out of scope; it predates the PR. A follow-up issue is worth filing.
• note internal/config/store_postgres_history_save.go:47 - FOR KEY SHARE needs UPDATE privilege on cloud_accounts. The app role already updates cloud_accounts (UpdateCloudAccount), so it is not a regression.
• note (permission change) internal/api/handler_marketplace.go:345-361 - host-account rows now carry the registered host account's UUID. authorizeSessionSell then admits sell-own and account-constrained grants for that account, which used to be a 403. loadMarketplaceAWSConfig (handler_marketplace.go:65-95) now signs through that account's resolver; role_arn with no ARN is accepted only for the host. That is consistent with the 000139 backfill (chore(db): backfill purchase_history.cloud_account_id from (provider, external_id) #1201). Registering an account is gated by create:accounts (handler_accounts.go:307), and (provider, external_id) is globally UNIQUE (000011:38), so there is no cross-provider or cross-tenant leak. No tenant column exists.
• nit process: the largest commit is ae3ebb1 at 235 lines, under the cap.

Attacked:

• Writers: the only INSERT INTO purchase_history is SavePurchaseHistory, with a single caller (purchase/execution.go:1024 via savePurchaseHistory, from execution.go:738). Direct, ambient, legacy and fan-out all converge there. Marketplace and reconciliation only UPDATE. Covered.
• Lookup: (provider, external_id) with LOWER(provider), exact external id and an empty id excluded matches the chore(db): backfill purchase_history.cloud_account_id from (provider, external_id) #1201 backfill rules. Cross-provider cases (aws/azure share a number) are tested.
• Dedupe: NOT EXISTS / ON CONFLICT plus RETURNING gives ErrNoRows, which is handled as "already recorded" (pgxmock test). An existing row is not rewritten (integration test).
• Merge-tree against origin/main 6776e27: clean (tree c3097386). go build ./... OK; go vet -tags integration OK.
• Merged tree: go test ./internal/config ./internal/purchase ./internal/api OK. -tags integration ./internal/config (full package) OK. -tags integration ./internal/api -run 'Marketplace|History|Purchase' OK, including resolved-account and missing-account.
• Mutation probes (integration, restored, verified with cmp):
  • drop FOR KEY SHARE: ConcurrentAccountDelete and ConcurrentSavesAndDelete FAIL;
  • drop LOWER(): legacy_upper-case_provider FAIL;
  • COALESCE replaced by $17 only: 5 stamp subtests plus ConcurrentAccountDelete FAIL.
• CI: Security Scanning and Build Docker Image are red on GO-2026-6809 (crypto/x509, go1.26.9). That comes from the old base c8bdd46; main is now on go 1.26.10. Not from the PR.

@IAmJSD CI on this head is red only from the old merge base (govulncheck on go1.26.9, fixed on main by #1236). Please merge or rebase onto current main so CI runs on a fresh merge ref.

[IAmJSD]

Merged current main (6b586cc) into the branch. The merge was clean, with no conflicts. New head: b064b330e3aeb2df494c767ef0e1fbbf5eaa2216.

The #1200 History summary fixtures. The summary tests now saw linked rows where they expected unlinked ones. Both summary fixtures seed "legacy" purchase_history rows by calling SavePurchaseHistory with a nil CloudAccountID while a registered account has the same external id. With this PR, those rows come back linked. The tests would still pass, but the drift guards would stop covering the unlinked shape (external account_id only), which is the shape they exist for. The History page fixtures from #1277 have the same pattern.

New commit b064b33 (test(history): keep legacy History fixture rows unlinked) fixes this:

• After each such save, the fixture sets cloud_account_id back to NULL.
• It requires that exactly one row was updated.
• It is applied in four fixtures: internal/api/handler_history_summary_integration_test.go, internal/config/store_postgres_history_summary_integration_test.go, and the two matching *_history_page_integration_test.go files.
• No assertion was changed. This is the same approach the PR already uses for TestMarketplacePermissionConstraintsHTTP.

Checks on the merged tree, with no cloud credentials:

• go build ./..., go vet ./..., go vet -tags integration ./... and the e2e vet all pass.
• golangci-lint v2.10.1 (with and without the integration tag) and gocyclo -over 10 are clean.
• go test -race -short passes on internal/{api,config}.
• The full go test -race -tags integration run passes on internal/api and internal/config (testcontainers Postgres).
• The secrets scan prints "clean; planted key flagged", and the allowlist test passes.

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• Issue fix(history): set cloud_account_id on every purchase_history writer #1235 asks for two things this PR doesn't do literally: "fail loudly rather than silently writing NULL", and "a test per writer". The PR stores NULL with a Warnf, and its tests are store-level, with rows shaped like each writer, rather than driving purchase.Manager per path. The reasoning holds up: refusing the row recreates the fix(api): skip plan disable on planned purchase delete when the plan spans out-of-scope accounts #621 audit gap, and ambient-only deployments legitimately have no registered account. I'd accept it, but it's a decision against the issue text, so the acting owner should confirm it. If they don't, the PR should say Refs #1235 instead of Closes.
• Behaviour change: a host-account row written by the ambient path now carries the registered account's UUID, so an account-constrained sell grant on that account now authorises marketplace actions on it (resolved-account case). That's correct, because the RI lives in that account and 000139 and fix(api): marketplace rows with a NULL cloud_account_id use the host's ambient credentials #560 already treat it that way, but it widens what a scoped grant can act on for new rows. Worth noting in the release notes.
• Existing fan-out race (explicit CloudAccountID plus account deleted mid-purchase gives an FK abort and a lost history row): the body flags this as out of scope. It deserves its own issue.
• (From an earlier review, confirmed here) internal/config/store_postgres_comprehensive_test.go's mockablePostgresStore.SavePurchaseHistory is a stale copy of the writer SQL, so the TestSavePurchaseHistory_* mock tests don't exercise the real statement. This predates the PR. The new pgxmock and integration tests cover the real SQL.

Attacked:

• SQL: (provider, external_id) is UNIQUE (000011:38) and no later migration drops it, so the scalar subquery can't return more than one row. LOWER() is applied to the parameter rather than the column, so the unique index is still used. An explicit $17 wins via COALESCE. The NOT EXISTS dedupe or ON CONFLICT path returns no row, which maps to the old "already recorded" branch. SavePurchaseHistory is the only INSERT INTO purchase_history in non-test code. No migration and no rolling-deploy concern.
• Fail-before: with origin/main's store_postgres.go restored and the new file removed, TestSavePurchaseHistory_StampsCloudAccountID failed 5 subtests and TestSavePurchaseHistory_ConcurrentAccountDelete failed. Separately, a mutant that drops FOR KEY SHARE fails ConcurrentAccountDelete with FK violation 23503, so the lock is actually tested.
• Ran: go build ./..., go vet ./... with and without -tags integration, and the e2e vet (all clean). go test -race -short -count=1 ./internal/config/ ./internal/api/ passed. go test -race -tags integration -count=1 ./internal/config/ passed (whole package, 499s). go test -race -tags integration -run 'TestMarketplacePermissionConstraintsHTTP|History' ./internal/api/ passed. golangci-lint v2.10.1 with and without integration tags reported 0 issues, and gocyclo was clean.

Body-stale:

• none

_{Reviewer: rC1. 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 b064b330e3aeb2df494c767ef0e1fbbf5eaa2216 merged onto main (83683307), together with #1244 in that order (combined tree aab9da0f, 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 b064b330, green CI, and the combined-tree verification above. Squash-merged at exactly b064b330e3aeb2df494c767ef0e1fbbf5eaa2216.