Skip to content

feat(plans): settle a failed-with-submission row so a managed plan can be deleted - #1241

Draft
IAmJSD wants to merge 6 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-999
Draft

IAmJSD wants to merge 6 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-999

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Root cause

Deleting an editor-managed plan is refused (409 managed_plan_has_live_executions) while any execution may hold or buy capacity. requireManagedDeleteTx counts a failed row without a no_submission proof as live, because its outcome is unknown and it can still be retried (a retry of a row detached from a deleted plan would buy with no plan). Nothing could ever change that row: failed rows are never pruned, and the retry link isn't consulted. So even after a retry of the row completed, the plan could never be deleted, only disabled.

Fix

As the issue asks, this adds an explicit, audited operator action and leaves delete without a flag. Delete still fails closed on an unknown outcome until a named person has accounted for it.

  • Migration 000143: purchase_executions.outcome_settled_at, outcome_settled_by (TEXT, so the audit survives user deletion and records the admin API key sentinel) and outcome_settlement (the complete fixed-format record), with a CHECK that the three are set together. 134 to 139 are taken by open PRs, and 140 to 142 by open PRs fix(recommendations): key stored recs, rec ID and purchase identity on RDS az_config (#408) #1239 and perf(api): page recommendations from an ID-ordered keyset scan instead of the full list #1240. Gaps are expected (previousMigrationVersion).
  • SettleFailedExecution (internal/config/store_postgres_execution_settle.go):
    • Accepts only a failed plan execution with no no_submission proof. Any other status, a no_submission row, an ad-hoc row or a row whose plan is gone gets 409.
    • Requires Acknowledge and a reason, and stores operator:settled_unknown_outcome: ack=true chain_tail=<id> tail_status=<status> reason="...".
    • Locks the plan FOR SHARE, then the row FOR UPDATE, then its retry-chain tail FOR UPDATE. That's the same plan-first order as DeletePurchasePlan and the editor command footprint. It re-checks plan_id under the locks.
    • Refuses while the chain may still buy: a tail outside the terminal allowlist (pending, running and so on), a failed tail that can still be retried (unknown outcome and not itself settled), or a chain past LadderChainMaxDepth.
    • An identical repeat by the same actor replays (200, replayed: true). A different one gets 409.
  • What settling does not do: it never changes status and never makes the row count as unbought. holdsCapacity, the ramp ledger (ListPlanCommittedTx), ramp_ledger_holds and the retention trigger are untouched, so a possibly-submitted purchase is never hidden or re-bought. The row stays failed in History after the plan is deleted (plan_id is set to NULL, as for completed rows today). RecoverReapedExecution can still record a late completion over it.
  • requireManagedDeleteTx skips a settled failed row (blocksManagedDelete). Every other classification is unchanged.
  • LinkRetryExecutionAtomic also requires outcome_settled_at IS NULL, so a settled row can never be retried. Retry's lost-race 409 now reads "already retried or settled; reload it".
  • POST /api/purchases/{id}/settle, body {"reason": "...", "acknowledge_unknown_outcome": true}:
    • Requires execute:purchases (admin:* alone is not enough, matching the separation-of-duties carve-out) and account access to the execution (out of scope gets 404).
    • Checks the grant's constraints with StrictScope against what the execution would have bought: its recommendations' providers, services, regions and total commitment, plus its accounts. Any dimension the execution leaves unknown requires an unconstrained grant.
    • The reason must be 10 to 300 characters with no control characters. Unknown fields are rejected.
    • Logs purchase execution settled by operator with the actor, the credential kind and the stored record.
  • known_issues/resolved/35_legacy_guard_followups.md: the feat(plans): settle a failed-with-submission row so a managed plan can be deleted #999 entry is marked resolved. Its other entry (fix(cleanup): give the cleanup lambda the live-status guard CleanupOldExecutions has #998) was handled by PR fix(cleanup): only delete settled executions when their TTL lapses #1173, which is now on main, so the doc's audit status reads 0 needs triage · 2 resolved and the doc moved to known_issues/resolved/ in the merge of main.

Closes #999

Verification

All of this ran locally on macOS against a real Postgres in Docker (testcontainers). It is unit and integration evidence, not real-cloud evidence. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

go build ./...
go test -tags integration -count=1 ./internal/config/                                   # ok (full package, 24s)
go test -tags integration -count=1 -run 'Retry|Managed|Settle|Delete|Reschedule|Resume' ./internal/api/   # ok
go test -tags integration -count=1 -run 'TestMigration_PurchaseExecutionsOutcomeSettlement' ./internal/database/postgres/migrations/  # ok
go test -count=1 ./internal/api/ ./internal/config/ ./internal/mocks/ ./internal/purchase/   # ok
go vet ./internal/api/ ./internal/config/ ./internal/database/...   (also with -tags integration)
golangci-lint v2.10.1 run --build-tags integration ./internal/api/... ./internal/config/... ./internal/database/...   # 0 issues (and without the tag)
gocyclo -over 10 -ignore "_test\.go" internal/api internal/config   # silent
bash scripts/gosec-hook.sh <changed .go files>   # clean
npx markdownlint-cli@0.47.0 known_issues/35_legacy_guard_followups.md   # clean (before the move to resolved/)
git-secrets (fresh repo, isolated HOME, changed files git-added, planted AKIA key flagged, clean once removed); scripts/test-git-secrets-allowlist.sh   # 137 PASS, 0 FAIL

New tests:

  • internal/config/store_postgres_execution_settle_integration_test.go
    • TestSettleFailedExecutionLetsManagedDeleteProceed reproduces the issue scenario: a failed row whose retry completed blocks the delete. After a settle, the delete proceeds, the row is still failed and not no_submission, the audit survives, and ListPlanCommittedTx returns exactly the same ledger before and after.
    • TestSettleFailedExecutionRefusals covers completed, pending, canceled, failed+no_submission, retry tails pending/running/failed, no plan, not found, and no ack/actor/reason. None of them writes anything.
    • TestSettleFailedExecutionChainOfUnknownOutcomes checks that the head is refused while its failed tail can still be retried, and that the delete stays blocked until both are settled.
    • TestSettleFailedExecutionReplayAndRetry covers the replay, the 409 on a different reason, and the retry CAS refusing a settled row.
    • Races, each queued on a real lock and asserted via pg_stat_activity:
      • TestSettleWaitsForRetryAndRefusesItsLiveSuccessor: a retry linked first; the settle waits on the row lock, then refuses the pending successor.
      • TestSettleWaitsForDeleteAndRefusesDetachedRow: a delete holding the plan lock commits first; the settle waits on FOR SHARE, then refuses the detached row.
      • TestDeleteWaitsForSettleAndSeesIt: a delete queued behind a settle sees the committed settlement.
  • internal/api/handler_execution_settle_integration_test.go
    • TestSettleUnblocksManagedPlanDeleteOverHTTP: delete 409, then POST /api/purchases/{id}/settle through HandleRequest (routing and CSRF) returns 200, then delete succeeds.
    • TestSettleAuthorizationAndValidation: admin without execute gets 403, out of account scope gets 404, execute constrained to another account or provider gets 403, a missing ack, short reason or unknown field gets 400, and a completed row gets 409. A refused settle writes nothing. Execute constrained to the plan's account succeeds.
  • Unit tests: TestBlocksManagedDelete, TestSettlementRecordReplayMatchesOnlyTheSameReason, TestDecodeExecutionSettle, TestMapExecutionSettleError, plus the migration up/down/CHECK test.

Fail-before / pass-after (mutation probes on this branch, each reverted afterwards):

Mutation Failing test(s)
guard ignores settlement (the pre-fix behavior) ...LetsManagedDeleteProceed, ...ChainOfUnknownOutcomes, TestDeleteWaitsForSettleAndSeesIt
retry CAS ignores settlement ...ReplayAndRetry
no plan_id re-check under the locks TestSettleWaitsForDeleteAndRefusesDetachedRow
no plan FOR SHARE lock TestSettleWaitsForDeleteAndRefusesDetachedRow
retryable failed tail allowed ...Refusals, ...ChainOfUnknownOutcomes
status check removed ...Refusals
handler skips constraints / account access / uses view instead of execute TestSettleAuthorizationAndValidation

On main, TestDeleteManagedPlanByExecutionStatus/failed already shows the permanent 409, and no operation existed that could clear it.

Coverage gaps: nothing here touches a real cloud account. The operator's cloud-side check stays a human step, recorded only as the free-text reason and the acknowledgement. The full internal/api integration package was not run, only the subset above.

Out of scope

  • The delete 409 still doesn't name the blocking execution ids. An operator finds them in History (failed rows of the plan). Adding blocking_execution_ids to the refusal details would be a small follow-up.
  • History/execution GET responses don't expose the settlement columns yet. The settle response and the server log carry them.
  • No structured cloud check (account/region/service/checked_at), unlike the ladder resolve, because a plan execution can span several services and regions. The reason field carries it.

Frontend follow-up for the UI owner

  • On a 409 managed_plan_has_live_executions from plan delete, explain that failed purchases with an unknown outcome must be settled first, and link to the plan's failed executions in History.
  • In History, for a failed plan execution that isn't no_submission, offer Settle next to Retry. It should open a confirm dialog that asks for a reason (10 to 300 characters) and a required "I checked the cloud account; this may have been bought, and it will never be retried" checkbox. It then sends POST /api/purchases/{id}/settle with {"reason", "acknowledge_unknown_outcome": true}. On 409, show the message (for example, a retry is still pending).
  • Once settled, hide Retry for that row. That needs the settlement fields on the History payload (see the out-of-scope note above).

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

Known unrelated CI failure: govulncheck (GO-2026-6809) until #1236 merges.

🤖 Generated with Claude Code

IAmJSD and others added 4 commits October 22, 2026 02:00
…loud#999)

Migration 000143 adds outcome_settled_at, outcome_settled_by and
outcome_settlement to purchase_executions, with a CHECK that they are set
together. They hold an operator's audited settlement of a failed execution
whose outcome is unknown. Settling never changes status or the ramp
ledger's view of the row.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lan can be deleted (LeanerCloud#999)

SettleFailedExecution records an operator's acknowledged, reasoned
settlement of a failed plan execution with no no_submission proof. It
locks the plan FOR SHARE, then the row and its retry-chain tail FOR
UPDATE, and refuses while the chain may still buy: a tail outside the
terminal allowlist or a failed tail that can still be retried. Replaying
the same request returns the stored settlement.

The row stays failed and keeps counting as possibly bought in the ramp
ledger. requireManagedDeleteTx skips a settled failed row, and
LinkRetryExecutionAtomic refuses one, since a retry of a row detached
from its deleted plan would buy with no plan.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ng races (LeanerCloud#999)

Covers the issue scenario (a failed row whose retry completed blocks the
managed delete until settled; the ledger is unchanged), every refusal,
chains of unknown outcomes, replay, the retry CAS, and the three
orderings that matter: a retry that links first, a delete that holds the
plan first, and a delete that queues behind a settle.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ilures (LeanerCloud#999)

The endpoint needs execute:purchases, account access to the execution
and the grant's constraints against what the execution would have bought
(strict scope), a 10-300 character reason and
acknowledge_unknown_outcome: true. It logs the settlement with the actor
and credential kind. Retry's lost-race 409 now says the row may have
been settled. Marks the known issue resolved.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

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.

@IAmJSD IAmJSD added priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged urgency/this-quarter Within the quarter impact/few Limited audience effort/m Days type/feat New capability labels Oct 22, 2026
Resolve the known_issues/35_legacy_guard_followups.md audit-status
conflict with LeanerCloud#1173: both entries (LeanerCloud#998 and LeanerCloud#999) are now resolved, so
the line reads "0 needs triage · 2 resolved" and the doc moves to
known_issues/resolved/, as the PR body said the second of the two to
merge would do.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator Author

Merged origin/main (at a14dc8f) to resolve the conflict in known_issues/35_legacy_guard_followups.md. There are no review findings on this PR yet, so the merge is the only change.

How it was resolved

git diff origin/main...HEAD lists the same 14 files as before the merge.

New head: ee85e18

Verification on the merged tree (Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null)

  • go build ./...; go vet of ./internal/api/ ./internal/config/ ./internal/database/... with and without -tags integration: clean
  • go test -count=1 of ./internal/api/, ./internal/config/, ./internal/mocks/, ./internal/database/postgres/migrations/ and ./internal/purchase/: ok
  • go test -tags integration -count=1 ./internal/config/: ok (232s). I skipped TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which already fails on main and is fixed in fix(config): compare resolve checked_at at the database's microsecond precision #1252.
  • ./internal/database/postgres/migrations/ integration: TestMigration_PurchaseExecutionsOutcomeSettlement and TestMigrations_DownAllRoundTrip pass. The full package does not finish within -timeout 20m on this shared machine (each migration test takes about 50s).
  • ./internal/api/ integration: every top-level test matching Retry|Managed|Settle|Delete|Reschedule|Resume|UpdateAny (38 tests) passes, including TestSettleUnblocksManagedPlanDeleteOverHTTP and TestSettleAuthorizationAndValidation. I ran them in batches to stay under 20m. Three subtests (TestDeleteManagedPlanByExecutionStatus/notified, /scheduled and TestRetry_RefusesSettledLadderTranche/completed) first failed to start their container (testcontainers reaper unexpected container status "removing" on the shared Docker host). All three passed on rerun. The full internal/api integration package was not run.
  • golangci-lint v2.10.1 on ./internal/api/... ./internal/config/... ./internal/database/..., with and without --build-tags integration: 0 issues
  • gocyclo -over 10 -ignore "_test\.go" on the changed non-test files: silent. scripts/gosec-hook.sh on them: clean.
  • git-secrets: fresh repo with an isolated HOME and the 14 changed files git-added: clean. A planted AKIA key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
  • Not run: markdownlint (not available offline here). Only the audit-status line and the file path of the Markdown doc changed.

Known unrelated CI failure: govulncheck (GO-2026-6809) until #1236 merges.

Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim

cristim commented Oct 25, 2026

Copy link
Copy Markdown
Member

Independent review 2 (claude-fleet, cold): REQUEST_CHANGES a190e55

Findings:

  • One blocking finding has been reported privately to the maintainers.
  • [non-blocking] internal/config/store_postgres_execution_settle.go:209 (replaySettlement) - No test covers the actor half of the replay check.
    • Removing *t.settledBy != req.Actor || (M4) passes every test.
    • If it regressed, a second operator sending the same reason would get 200 replayed:true instead of the promised 409.
    • Nothing is written, so the impact is audit confusion only.
  • [non-blocking] settle.go:168 (lockSettleChainTail) - No test covers the retryID != nil "chain changed while settling" refusal (M7 survives).
    • The window is narrow, because only the tail is locked.
    • Losing this check would let a settle record a stale chain_tail while a newer successor is live.
  • [non-blocking] settle.go:21/108 vs handler_execution_settle.go:20/151 - The handler allows up to 300 characters, but the store allows up to 1000 bytes.
    • A 300-character reason in 4-byte runes (1200 bytes) passes the handler and then gets a 400 that says "1 to 1000 bytes".
    • The two limits should use the same unit.
  • [non-blocking] settle.go:149 (checkSettleTarget) - Settle works on a failed row of any plan, not only an editor-managed one (DatesManaged).
    • On a non-managed plan it unblocks nothing, and only removes Retry permanently.
    • Restrict it, or state in the PR that this is intended.

Attacked:

  • The settle transition runs in one transaction. The lock order is:
    1. the plan FOR SHARE;
    2. the row FOR UPDATE, with a plan_id re-check;
    3. the tail FOR UPDATE;
    4. a CAS UPDATE WHERE outcome_settled_at IS NULL.
    • Status, no_submission, settledAt and the tail are all judged under those locks.
    • Retry: LinkRetryExecutionAtomic is the only writer of the retry link (store_postgres.go:1201, with one caller at handler_purchases.go:2633). Under READ COMMITTED it re-evaluates outcome_settled_at IS NULL after the settle commits, so whichever side commits first wins.
    • Re-drive only claims approved rows (manager.go:620).
    • The reaper only CASes running and approved rows. Its follow-up annotation (reaper.go:283) and RecoverReapedExecution both go through SavePurchaseExecutionTx, whose DO UPDATE SET covers neither plan_id nor the settlement columns (store_postgres.go:1096-1120). So a late completion over a settled, detached row still records the money and keeps the audit stamp.
    • Ladder executions have no plan_id, so settle refuses them (409).
  • Outcome recording:
    • Status and no_submission are untouched.
    • holdsCapacity, ramp_ledger_holds and the retention trigger are unchanged.
    • DeletePurchasePlan drops suppressions only for pending, notified and paused rows (store_postgres.go:900-910). So a settled failed row keeps its suppressions and its ledger spend.
    • The test asserts the ledger is identical before and after the settle.
  • The delete gate: blocksManagedDelete skips only a settled failed row. Unsettled failed rows, including the chain head, keep blocking (ChainOfUnknownOutcomes).
  • Migration 000143:
  • Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
    • M1, the guard ignores settlement: 4 tests FAIL.
    • M2, the retry CAS ignores settlement: ReplayAndRetry FAILS.
    • M3, a retryable tail allowed: Refusals and Chain FAIL.
    • M4, replay ignores the actor: survives (gap above).
    • M5, no plan FOR SHARE: WaitsForDelete FAILS.
    • M6, no_submission allowed: Refusals FAILS.
    • M7, the chain-changed check off: survives (gap above).
    • M8, the handler skips constraints: AuthorizationAndValidation FAILS.
  • Fail-before: on base, the new store tests do not compile, and TestDeleteManagedPlanByExecutionStatus/failed passes, which confirms the permanent 409.

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

  • go build ./... and go vet on api, config and database, with and without -tags integration: OK. gofmt: clean.
  • go test -count=1 on api, config, mocks, purchase and migrations: ok.
  • Integration tests: ok.
    • config: Settle|ManagedDelete|LinkRetry|RecoverReaped.
    • api: TestSettle|TestDeleteManagedPlanByExecutionStatus|TestRetry.
    • migrations: TestMigration_PurchaseExecutionsOutcomeSettlement (up/down/up) and TestMigrations_DownAllRoundTrip.
    • All 7 store settle tests and both HTTP settle tests pass.
  • git merge-tree --write-tree origin/main <head>: clean.

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

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

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

Blocking:

  • none

Non-blocking:

  • Merge conflict with main. The PR is CONFLICTING. git merge-tree shows one conflict, in internal/api/handler_purchases.go: main's chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217 moved the body of persistRetrySuccessorTx into writeRetrySuccessorTx. The resolution should carry the "already retried or settled" 409 text into main's new location. The authoritative guard is the outcome_settled_at IS NULL predicate in LinkRetryExecutionAtomic (store_postgres.go), which merges cleanly. The new head needs a fresh review.

  • Migration 000143 up (000143_purchase_executions_outcome_settlement.up.sql:19): ADD CONSTRAINT ... CHECK validates every row while holding ACCESS EXCLUSIVE on purchase_executions. All rows are NULL, so it passes, but it is a full scan under the strongest lock. ADD CONSTRAINT ... NOT VALID followed by VALIDATE CONSTRAINT would avoid that if the table is large in any deployment. The number (143) matches the reservation for feat(plans): settle a failed-with-submission row so a managed plan can be deleted #1241. Up, down and re-up are covered by the migration test and pass.

  • Rolling deploy. Until every instance runs the new LinkRetryExecutionAtomic, an old instance could still retry a row that a new instance just settled. The window is small and requires both actions at once. Noting it, not blocking.

  • Reason length mismatch (internal/api/handler_execution_settle.go:20 vs internal/config/store_postgres_execution_settle.go:21): the handler allows 300 runes, but the store caps the reason at 1000 bytes. A 300-character reason of 4-byte runes (e.g. emoji or CJK extension characters) passes the handler and then gets the store's 400 "reason must be 1 to 1000 bytes". It's harmless, but the two limits should agree.

  • No 4-eyes on settle. One execute:purchases holder can settle a row, which permanently retires it from retry and lets the managed plan be deleted. This moves no money, but it is a policy choice. I've flagged it for the acting owner.

  • History doesn't show the settlement yet (out of scope, per the body). The UI will still offer Retry on a settled row, and that click returns the new 409.

  • (From an earlier review, which I agree with.) checkSettleTarget accepts a failed row of any plan, not only an editor-managed one. On a non-managed plan, settling unblocks nothing and only removes Retry for good. Either restrict it to dates_managed plans or say in the PR that this is intended.

  • (From an earlier review.) No test covers the actor half of replaySettlement (*t.settledBy != req.Actor) or the "retry chain changed while settling" refusal in lockSettleChainTail. Each is a one-test gap.

  • An earlier review on this same head (cristim, 2026-10-25) says one blocking finding was reported privately to the maintainers. I could not see it, so this approval does not cover it. That finding needs to be resolved before merge.

Attacked:

  • Money/guard paths. I read requireManagedDeleteTx/blocksManagedDelete: only an operator-settled failed row stops blocking, and every other classification is unchanged. holdsCapacity, the ramp ledger and the retention paths are untouched, which the integration test asserts (ListPlanCommittedTx unchanged across the settle).
  • Other ways out of failed. I grepped for paths that move a row out of failed: only the retry CAS and RecoverReapedExecution (failed to completed, which is harmless), so the CAS predicate is enough to stop a settled row from being bought again.
  • Retry atomicity. The retry inserts the successor and links it in one tx, so a settled row makes the link match 0 rows and the successor is rolled back.
  • Lock order. plan FOR SHARE, then row FOR UPDATE, then tail FOR UPDATE. This is compatible with the delete (plan lock first) and the retry (row only), and the three race tests queue on real locks.
  • Authorization. execute:purchases (admin alone is refused), requireExecutionAccess, then StrictScope constraint sets with the execution's accounts. Ad-hoc rows, no_submission rows and rows whose plan is gone are refused.
  • Fail-before. In my scratch checkout I reverted blocksManagedDelete's settled branch and the CAS predicate, then ran -tags integration -run 'TestSettle|TestDeleteWaitsForSettle' ./internal/config/. Four tests failed: LetsManagedDeleteProceed, ChainOfUnknownOutcomes, ReplayAndRetry and TestDeleteWaitsForSettleAndSeesIt. I restored the PR's code afterwards.
  • Commands. go build ./...; go vet with and without -tags integration on internal/{api,config,database/...}; go test -race -short -count=1 on the same packages, ok; go test -race -tags integration -count=1 on ./internal/config/ (180s), ./internal/database/postgres/migrations/ (177s) and the full ./internal/api/ (258s), all ok; golangci-lint v2.10.1 with and without the integration tag, 0 issues; gocyclo -over 10 on the changed non-test files, silent.

Body-stale:

  • "134 to 139 are taken by open PRs": 134 to 139 are on main now (main is at 000139).
  • "the doc moved to known_issues/resolved/ in the merge of main": on main the doc is still at known_issues/35_legacy_guard_followups.md. This PR is what moves it.
  • "Known unrelated CI failure: govulncheck (GO-2026-6809) until fix(deps): bump Go to 1.26.10 for GO-2026-6809 #1236 merges": fix(deps): bump Go to 1.26.10 for GO-2026-6809 #1236 has merged.
  • "The full internal/api integration package was not run": I ran it at this head and it passes.

Reviewer: rB27. 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 added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 26, 2026
…ation 000145 -> 000141

golang-migrate only applies versions above the database's current one,
so PRs must merge in migration-number order. 000140-000144 had been
taken by PRs that are now held on owner decisions (LeanerCloud#1239, LeanerCloud#1241,
LeanerCloud#1253). The ready PRs move to 000140-000145 so they can merge ahead of
them, and the held ones will take the next free numbers when decided.
Rename only: the SQL statements are unchanged, and the test version
constants and the comments naming the migration follow the new number.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 26, 2026
…000140

golang-migrate only applies versions above the database's current one,
so PRs must merge in migration-number order. 000140, 000141, 000143 and
000144 belong to PRs held on owner decisions (LeanerCloud#1239, LeanerCloud#1241, LeanerCloud#1253); this
PR takes 000140 so it can merge ahead of them, and they will take the
next free numbers when decided. Rename only: the SQL is unchanged, and
the test's version constant and the comments naming the migration
follow the new number.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 26, 2026
…7 -> 000143

golang-migrate only applies versions above the database's current one,
so PRs must merge in migration-number order. 000140-000144 had been
taken by PRs that are now held on owner decisions (LeanerCloud#1239, LeanerCloud#1241,
LeanerCloud#1253). The ready PRs move to 000140-000145 so they can merge ahead of
them, and the held ones will take the next free numbers when decided.
Rename only: the SQL statements are unchanged, and the test version
constants and the comments naming the migration follow the new number.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 26, 2026
…-> 000145

golang-migrate only applies versions above the database's current one,
so PRs must merge in migration-number order. 000140-000144 had been
taken by PRs that are now held on owner decisions (LeanerCloud#1239, LeanerCloud#1241,
LeanerCloud#1253). The ready PRs move to 000140-000145 so they can merge ahead of
them, and the held ones will take the next free numbers when decided.
Rename only: the SQL statements are unchanged, and the test version
constants and the comments naming the migration follow the new number.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 26, 2026
…148 -> 000144

golang-migrate only applies versions above the database's current one,
so PRs must merge in migration-number order. 000140-000144 had been
taken by PRs that are now held on owner decisions (LeanerCloud#1239, LeanerCloud#1241,
LeanerCloud#1253). The ready PRs move to 000140-000145 so they can merge ahead of
them, and the held ones will take the next free numbers when decided.
Rename only: the SQL statements are unchanged, and the test version
constants and the comments naming the migration follow the new number.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Migration renumbering needed before this merges. The reviewed, ready PRs have taken 000140-000145 so they could merge without waiting on owner decisions: #1240 (000140), #1302 (000141), #1268 (000142), #1287 (000143), #1289 (000144), #1116 (000145). This PR's migration(s) (000143) must be renumbered to start at 000146, or the next free number at that time, when it's ready to merge (000143 -> 000146 or the next free number). golang-migrate skips a version below the database's current one, so a lower number merged after those would never apply.

Dependency check: none of the six ready migrations touches purchase_executions, so the order doesn't matter beyond the number. That ordering hasn't been run.

IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 26, 2026
…n 000146 -> 000142

golang-migrate only applies versions above the database's current one,
so PRs must merge in migration-number order. 000140-000144 had been
taken by PRs that are now held on owner decisions (LeanerCloud#1239, LeanerCloud#1241,
LeanerCloud#1253). The ready PRs move to 000140-000145 so they can merge ahead of
them, and the held ones will take the next free numbers when decided.
Rename only: the SQL statements are unchanged, and the test version
constants and the comments naming the migration follow the new number.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit that referenced this pull request Oct 26, 2026
…d of the full list (#1240)

* fix(api): add opt-in keyset pagination to GET /api/recommendations

The list endpoint returned the whole filtered recommendations cache in a
single body, so a tenant with a few thousand cached rows pushed the
response past the 6 MB Lambda payload cap and the page failed with 502.
Gzip (#775) raised that ceiling but did not bound it.

Callers can now send limit (1-1000) and the returned next_cursor to read
the list in pages ordered by recommendation ID. The cursor is an opaque
base64url encoding of the last ID seen, so a row deleted between requests
does not shift later pages. summary and regions still describe the full
filtered set on every page, and compute capacity is only decoded for the
returned rows. Requests without limit or cursor keep the existing
unpaginated response, so the current frontend is unaffected until it
adopts the parameters. openapi.yaml documents the parameters and the
actual response shape.

Refs #777

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(api): cap recommendation pages by serialized bytes and reject empty paging params

A page was bounded only by row count, but the opaque Details blob makes a
row's size data-dependent, so 1000 large rows could still pass the 6 MiB
Lambda response cap. Pages now also stop once the rows, measured as they
appear inside the Lambda response envelope (the body re-encoded as a JSON
string, so quotes and backslashes count twice), plus the summary, regions,
keys and next_cursor would pass a 4 MiB budget. The budget is checked
against uncompressed bytes because the #775 gzip only runs when the client
accepts it. A page always holds at least one row, so a row that alone is
over the budget is returned on its own page with a logged warning instead
of stalling the cursor walk.

Legacy unpaginated mode is now selected only when neither limit nor
cursor is present. A present but empty ?limit= or ?cursor= returns 400
instead of falling back to the unbounded response.

Refs #723

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(api): reject duplicate recommendation IDs when paging and document 400s

- Correct the ordering comment: the paging key is the application
  recommendation ID built from the natural-key columns, not the cache
  table's UUID primary key. A paginated request now fails if two rows
  share an ID, since the strict "after" comparison would skip one of
  them at a page boundary.
- Describe the feature as opt-in in-memory cursor slicing, not a SQL
  keyset (store-side paging is #981).
- TestGetRecommendations_UnpaginatedShapeUnchanged copies the expected
  IDs before the call so an in-place reorder in legacy mode fails it.
- Add TestGetRecommendations_PaginationRejectsDuplicateIDs.
- openapi: document the 400 response for bad limit/cursor/filter
  values, and say next_cursor's absence is the only end-of-list signal.

Refs #723

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* perf(db): index recommendations by byte-wise payload id for keyset scans

Migration 000142 adds recommendations_scan_key_idx on
(COALESCE(payload->>'id', '') COLLATE "C", id). The paginated
GET /api/recommendations scan (issue #981) walks the cache in chunks in
that order and resumes each chunk after the last (id, primary key) it
read; without the index every chunk is a full scan plus a sort.

COLLATE "C" is byte order, the same order Go's string comparison uses,
so SQL paging keeps the order the API already returns.

Refs #981

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* perf(config): read stored recommendations in keyset chunks

ListStoredRecommendationsChunk returns at most limit rows matching a
RecommendationFilter, ordered by the byte-wise recommendation ID and then
the table's UUID primary key, resuming strictly after a given position.
The primary key keeps rows that share an ID in a total order, so a chunk
boundary inside such a run neither skips nor repeats a row. The
MinSavingsPct floor stays in-process (shared with
ListStoredRecommendations via decodeStoredRecommendation); rows it drops
still advance the position.

Integration tests pin the order to Go's sort.Strings on IDs that a
linguistic collation orders differently, cover duplicate IDs across every
chunk size, the pct floor emptying a whole chunk, the empty table, an
EXPLAIN check that recommendations_scan_key_idx serves the query without
a Sort node, and walks racing concurrent upserts.

Refs #981

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* perf(scheduler): scan cached recommendations chunk by chunk

ScanRecommendations walks the recommendations cache through
ListStoredRecommendationsChunk in byte-wise ID order and applies the same
suppressions and account overrides as ListRecommendations to each chunk,
handing the survivors to a callback. It holds one chunk at a time instead
of the whole filtered set. Suppressions are read once per scan and each
(account, provider, service) override is resolved once per scan, not once
per chunk. Read failures over-show exactly like ListRecommendations.

The freshness read, cold-start kick and stale-while-revalidate kick are
pulled out of ListRecommendations into startCacheRead/finishCacheRead so
both reads share them unchanged.

Tests pin the scan to ListRecommendations' output (sorted by ID) for chunk
sizes 1 to 31 with suppressions and overrides active.

Refs #981

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* perf(api): page GET /api/recommendations from the chunked scan

A paginated request (limit or cursor sent) no longer loads the whole
filtered set through ListRecommendations and sorts it in memory. It runs
Scheduler.ScanRecommendations, applies the enabled-providers and account
scope filters to each chunk (their inputs read once per request), and
feeds the survivors to recommendationsPageCollector. The collector totals
every row into the summary and keeps only the rows after the cursor up to
limit+1 (the page plus one row proving a next page exists), then cuts the
page with the existing byte budget. Peak memory is one scan chunk plus
one page instead of the whole tenant.

Output is unchanged: rows arrive in byte-wise ID order (Go string order),
duplicate IDs still fail the request, and the summary still covers the
full filtered set. The unpaginated path is untouched.

The in-memory sortRecommendationsAfter/buildBudgetedPage are removed;
the existing #723 pagination tests now run against the scan path, with
buildBudgetedPage kept as a test helper over the collector.

Refs #981

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(api): pin paginated recommendations order, ties and last page

- PaginatedRequestDoesNotLoadFullList: the #981 regression; a paged
  request must use ScanRecommendations, never ListRecommendations.
- PagedWalkMatchesLegacyOrder: for an unrestricted and a scoped reader,
  every chunk size and page size (including the exact-fit last page), the
  walk returns the legacy rows sorted by Go string order in consecutive
  pages, next_cursor on all but the last page, and the legacy summary on
  every page. IDs mix case, "|", "-", "/", spaces and accents.
- PagedDuplicateIDs: ties fail every paged request at any chunk/page
  boundary; a twin removed by the scope filter is not a tie.
- Background-collection scope (#627), the one-page memory bound, the
  out-of-order guard and the empty cache.
- An integration test walks a real Postgres cache spanning several scan
  chunks with the min_savings_pct floor and matches the legacy output.

Refs #981

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(scheduler): scan with main's variant-aware suppression index

Merging main brought #1110 (issue #816), which changed
aggregateSuppressions to return the raw rows grouped by 6-tuple
(map[suppressionKey][]config.PurchaseSuppression) and moved the per-rec
aggregate into applySuppressionIndex, because which rows apply now depends
on the rec's variant (AZ, platform, tenancy). ScanRecommendations still
held the old map[suppressionKey]*suppressionAgg, so the merged tree did not
build.

loadSuppressionIndex now returns main's grouped index and the scan passes
it to the same applySuppressionIndex as ListRecommendations. Matching is
per rec, so applying it chunk by chunk suppresses exactly what the non-scan
path does.

A new test pins the scan to ListRecommendations at every chunk size on an
EC2 fixture with a variant purchase, a fully covered variant, a legacy
wildcard row and a rec with unreadable Details, and checks the expected
per-variant counts. Stripping variants from the scan's index fails it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* chore(db): renumber recommendations_scan_key_idx migration 000142 -> 000140

golang-migrate only applies versions above the database's current one,
so PRs must merge in migration-number order. 000140, 000141, 000143 and
000144 belong to PRs held on owner decisions (#1239, #1241, #1253); this
PR takes 000140 so it can merge ahead of them, and they will take the
next free numbers when decided. Rename only: the SQL is unchanged, and
the test's version constant and the comments naming the migration
follow the new number.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Cristian Magherusan-Stanciu <cristim@users.noreply.github.com>
IAmJSD added a commit that referenced this pull request Oct 26, 2026
…vice (#1302)

* fix(purchase,server): skip the $0 purchase confirmation and report a failed lazy DB init on /health

An execution with nothing selected never reaches a cloud API, but both the
single-account and the per-account fan-out paths still sent a "purchase
confirmed" email listing zero commitments. Gate the confirmation on
anyRecPurchased / committed, so it goes out only when a real commitment
was created (full or partial success).

When reinitializeAfterConnect fails after setting app.Config, ensureDB
closes the pool and clears app.DB but leaves app.Config, and the
config_store health check reported healthy over the closed pool. Snapshot
dbConnected with the store pointers under dbMu and report unhealthy while
lazy PostgreSQL init has not completed.

Audit findings A05-015 and A06-009. A06-019 (commitment_type labels)
follows separately.

Refs #339

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(analytics): label snapshot commitment types from provider and service

commitmentTypeFor matched only a service literally named "SavingsPlans",
which purchase_history never stores, so every commitment was stored as
"RI": AWS Savings Plans ("savingsplans" / "savings-plans-<type>") and GCP
committed-use discounts included. Derive the type from (provider, service)
in the pkg/common CommitmentType vocabulary, and widen the
savings_snapshots CHECK in migration 000145 to match. The CHECK still
admits the legacy 'RI' / 'SavingsPlan' literals so an old replica can
write during a rolling deploy; existing rows are relabeled.

Audit finding A06-019.

Refs #339

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(purchase): make the #339 no-confirmation assertions checkable by the mock guard

TestNoUnfailableMockAssertions could not resolve the receiver of two
AssertNotCalled sites (a mock returned by a helper). Declare the mock
directly in the new test, and in the approve test rely on the missing
SendPurchaseConfirmation expectation, which fails the test on any send.

Refs #339

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(analytics): restore the pkg/common import after #1254

#1254 (on main) replaced the hand-rolled config-store mock in
collector_test.go and dropped the now-unused pkg/common import. This
branch's new commitment-type assertion uses common.CommitmentType, so the
analytics test build broke after main was merged in although the merge
was textually clean. Same assertion.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* chore(db): renumber savings_snapshots_commitment_type_vocabulary migration 000145 -> 000141

golang-migrate only applies versions above the database's current one,
so PRs must merge in migration-number order. 000140-000144 had been
taken by PRs that are now held on owner decisions (#1239, #1241,
#1253). The ready PRs move to 000140-000145 so they can merge ahead of
them, and the held ones will take the next free numbers when decided.
Rename only: the SQL statements are unchanged, and the test version
constants and the comments naming the migration follow the new number.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit that referenced this pull request Oct 26, 2026
…ail-closed plan providers, revocation_in_flight clear (#1268)

* fix(config/db): audit findings A04-010/011/012/013 in migrations and config store

- 000059 down: only remove ...0005 when it really is Purchaser. 000057 gave
  that UUID to Standard Users, so the old down deleted Standard Users and
  stripped it from every member. Refuse with a clear error rather than leave
  a Purchaser-only user with no group. 000096 down gets the same up-front
  refusal instead of a bare users_min_one_group violation.
- 000027: dedupe savings_snapshots on (tableoid, ctid). ctid is per
  partition, so matching on it alone deleted unrelated rows of other
  partitions on a forced re-run.
- DerivePlanProviders: accept the "provider:service" keys 000040 wrote, and
  return an error instead of an empty provider set when no key names a
  provider, so both plan-account guards fail closed.
- MarkPurchaseRevoked clears revocation_in_flight, as 000072 documents;
  migration 000146 clears it on rows already revoked.

Real-Postgres tests for each, using the shared container. Normalizing the
plan-services key delimiter is deferred (see the PR).

Refs #316

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(config): identify legacy-keyed plan services by their provider field

The first #316 commit made DerivePlanProviders fail when no service key
had a provider prefix. That refused supported plans: a legacy alias kept
under its original key (#1123's singleton-alias edit path, exercised by
TestLegacyServiceB1S1*) has a key like "original-key", and so do fixtures
keyed "ec2" with Provider "aws".

Each entry's provider now comes from its key prefix ("provider/service"
or "provider:service"), falling back to the entry's own Provider field,
which is what mergeRequestedServices and plan step resolution use. The
check fails closed only when no entry identifies a provider either way.

Refs #316

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(migrations): drop the duplicate TestMain from helpers_test.go

main's #1400 added main_integration_test.go with a TestMain that stops
the shared PostgreSQL container after the run. This branch added an
identical TestMain to helpers_test.go, so after merging main the
integration build of the package declared TestMain twice and failed to
compile. Keep main's TestMain, which does the same teardown, and
restore helpers_test.go to main's version.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* chore(db): renumber purchase_history_clear_revoked_in_flight migration 000146 -> 000142

golang-migrate only applies versions above the database's current one,
so PRs must merge in migration-number order. 000140-000144 had been
taken by PRs that are now held on owner decisions (#1239, #1241,
#1253). The ready PRs move to 000140-000145 so they can merge ahead of
them, and the held ones will take the next free numbers when decided.
Rename only: the SQL statements are unchanged, and the test version
constants and the comments naming the migration follow the new number.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit that referenced this pull request Oct 26, 2026
…ngs rollups (#1287)

* fix(analytics): keep unknown usage and commitment unknown in the savings rollups

savings_snapshots.total_usage and total_commitment are nullable on purpose:
NULL means the provider gave no figure. monthly_savings_summary and
provider_savings_summary rolled them up with SUM and AVG, which skip NULL,
so "us-east-1 $4000 + eu-west-1 unknown" became a definite $4000.

Migration 000147 recreates both views so a timestamp's total is NULL when
any of its rows is unknown, and the period average is NULL when any of its
timestamps is. A known 0 stays 0. Column names and types, the unique
indexes and the CONCURRENTLY refresh path are unchanged; the down migration
restores the previous definitions.

Integration tests on real Postgres cover unknown, known zero, fully known,
unknown-at-one-instant, down/up, and a refresh_savings_materialized_views()
call queued behind the migration's locks.

Refs #151

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* chore(db): renumber savings_rollups_propagate_unknown migration 000147 -> 000143

golang-migrate only applies versions above the database's current one,
so PRs must merge in migration-number order. 000140-000144 had been
taken by PRs that are now held on owner decisions (#1239, #1241,
#1253). The ready PRs move to 000140-000145 so they can merge ahead of
them, and the held ones will take the next free numbers when decided.
Rename only: the SQL statements are unchanged, and the test version
constants and the comments naming the migration follow the new number.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit that referenced this pull request Oct 26, 2026
…nd AWS fee (backend slice of #27) (#1289)

* feat(marketplace): add listing settlement columns and guarded poller writes

Migration 000148 adds the purchase_history settlement columns that 000087
deferred (listed_at, listing_proceeds_received, listing_fee_paid).
listing_price_schedule already exists via 000105 (#525) and is left alone,
including on rollback.

The store gains the three statements the RI Marketplace status poller
needs: the scan of listings to poll and two status-guarded UPDATEs. The
guards (same listing id, active or same terminal state, proceeds not yet
recorded) decide whether a poll result applies. Concurrent polls therefore
record proceeds once, and a cancel recorded by the cancel handler is never
revived or turned into a sale. Real-Postgres tests cover the races.

Refs #27

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* feat(marketplace): poll RI Marketplace listings for sales and fees

PollMarketplaceListings reads each listing due for a poll from AWS
(DescribeReservedInstancesListings through the existing listing-finder
interface, signed as the purchase's own account like list and cancel).
While a listing is active it records its CreateDate as listed_at. When
the listing closes, or is canceled after a partial sale, it records the
terminal state with the seller's net proceeds and the AWS fee: the
instances AWS reports sold, times the active price tier, minus the
12% fee, in whole cents. When instances sold but no single active USD
tier exists, the state is recorded and the proceeds are left unset for
a later poll instead of guessed. A failure on one listing is counted
and the run moves on to the next.

Refs #27

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(api): check poller non-writes through MockConfigStore's call log

The vacuous-assertion guard cannot resolve f.store as a receiver. Give
the two tests their own *MockConfigStore and use the name-only
AssertNotCalled, which reads the call log and fails on any call.

Refs #27

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* feat(server): run the marketplace listing poll in the finalize_revocations sweep

The finalize_revocations task now also runs the RI Marketplace status
poller after the revocation repair. Both reconcile purchase_history with
the provider on the same hourly schedule, so no new schedule or
infrastructure is needed. The two steps are independent: one failing
does not skip the other, and the task fails if either did. The task's
result keeps the revocation counts under their existing keys and adds
the poll counts under marketplace_listings.

Refs #27

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* chore(db): renumber purchase_history_listing_settlement migration 000148 -> 000144

golang-migrate only applies versions above the database's current one,
so PRs must merge in migration-number order. 000140-000144 had been
taken by PRs that are now held on owner decisions (#1239, #1241,
#1253). The ready PRs move to 000140-000145 so they can merge ahead of
them, and the held ones will take the next free numbers when decided.
Rename only: the SQL statements are unchanged, and the test version
constants and the comments naming the migration follow the new number.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(config): drop the previous listing's settlement when a relist is recorded

Repricing an RI on the Marketplace means cancel and relist, since AWS has no
price edit. A relisted row kept the earlier listing's listed_at, proceeds and
fee, and only RecordMarketplaceListingActive cleared them, which needs a poll
to see the new listing active first. A relist that sold out (or was canceled)
before the next hourly poll was therefore never settled: the settle guard
refuses rows whose proceeds are set, so the row stayed active with the old
0.00 and was polled and skipped every hour, or showed the old listing's
settlement as the new one's.

UpdatePurchaseHistoryListing and ResolveMarketplaceListingAttempt, the two
statements that write a listing id, now null the three columns in the same
UPDATE when the id changes. Re-recording the id the row already holds (a
cancel or its retry) keeps them. The new real-Postgres test runs the review's
exact sequence plus the canceled-within-the-hour and resolved-attempt
variants.

Refs #27

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit that referenced this pull request Oct 26, 2026
* fix(auth): bind group account scope entries to account IDs

A group's allowed_accounts could hold an account's display name. The name
was not bound to any account, so once that account was deleted or renamed
the entry matched whichever account was later created or renamed to the
same name. #1032 limited name matching to unique names, which does not
help here: the newcomer is the only account carrying the name.

Scope entries now match by account identifier only:

- AccountScope.Allows never compares an entry with a display name. A
  caller that knows an account only by name (legacy rows keyed by external
  ID) still resolves through WithRegisteredAccounts to the one account
  carrying that name, and that account's ID must be in the scope.
- Group create/update bind allowed_accounts at grant time: "*", account
  IDs and external IDs are kept; a name exactly one registered account
  carries, inside the actor's own scope, is stored as that account's ID;
  anything else is a 400. A failed account or scope load refuses the write.
- Migration 000135 rewrites existing name entries to IDs when exactly one
  account carries the name and it predates the group's last write. Other
  entries are left in place and no longer grant access. Down is a no-op.
- The name-only MatchesAccount and AuthContext.CanAccessAccount helpers,
  which had no production callers, are removed.

Closes #1034

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(api): scope the history-summary fixture by account ID

The history-summary integration tests from #1200 scoped their user with
the tenant's display name. This branch binds allowed_accounts entries to
account IDs and makes a bare name entry match nothing (#1034), so the
scoped user saw no tenant A rows and the totals and listed-rows checks
failed. Grant the scope by tenant A's account ID, the form group writes
now store. The assertions are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* chore(db): renumber the group scope binding migration to 000149

main now ships migrations up to 000139, so 000135 would sit below
versions a deployed database has already applied and golang-migrate
would never run it. Open PRs hold 000140-000148, so this takes 000149
and must merge after that chain. The SQL is unchanged; only the file
names, header comments, test version and test fixture IDs move.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* chore(db): renumber bind_group_account_names_to_ids migration 000149 -> 000145

golang-migrate only applies versions above the database's current one,
so PRs must merge in migration-number order. 000140-000144 had been
taken by PRs that are now held on owner decisions (#1239, #1241,
#1253). The ready PRs move to 000140-000145 so they can merge ahead of
them, and the held ones will take the next free numbers when decided.
Rename only: the SQL statements are unchanged, and the test version
constants and the comments naming the migration follow the new number.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(api): register the wire-format group account for grant-time binding

TestUserGroupWrites_WireFormat came in from main (#1261) and builds a
Handler with no config store. Since #1034, group writes bind every
allowed_accounts entry to a registered account and refuse the write when
the account list cannot be loaded, so POST /api/groups answered 500.
The test now registers its account ID in a store mock, as the binding
tests do; the wire format it pins is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(api): scope the History paging user by account ID, not name

TestIntegration_HistoryPaging_ScopedUserNeverPagesIntoAnotherTenant came
in from main (#1277) and grants the scoped user tenant A by its display
name. Since #1034 an allowed_accounts entry matches by cloud account ID
only, so that user saw none of tenant A's rows and the walk returned
only its own unscoped execution. The grant now names tenant A's ID, as
the other scope tests on this branch do; what the test asserts is
unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(analytics): scope account_ids fixtures by account ID

The account_ids analytics tests that arrived with the main merge (#1448)
grant scope by account name ("a", "b", "c"). This branch binds group
scope entries to account IDs and no longer matches an entry against a
display name (#1034), so those name entries authorize nothing and the
scoped cases got 404. List the account IDs in the scope instead, as the
other analytics scope tests on this branch already do. The assertions
are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(analytics): scope the account_ids DB test by account ID

TestAnalyticsAccountIDs_SumsSelectedAccountsOnly_DB (from #1448) grants
the scoped user ["a", "b"], which are account names. Scope entries are
account IDs on this branch (#1034), so the name entries allowed nothing
and the in-scope query returned 404. Use the created accounts' IDs.
The foreign-id refusal assertion is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(history): scope the plan_id filter DB test by account ID

TestIntegration_HistoryPlanFilter_CrossTenant (from #1445) grants the
scoped user tenant A by account name. Scope entries are account IDs on
this branch (#1034), so the name allowed nothing and tenant A's own
plan came back empty. Grant tenant A's ID, as the other history
integration tests on this branch do. The cross-tenant assertions are
unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(api): scope #1464's actionable History tests by account ID

#1464 landed on main with tests that scope the user by the display name
"dev" and with an executionScope case built on WithRegisteredNames. This
branch (#1034) makes name entries grant nothing and replaces that
constructor, so after merging main those tests no longer compiled or no
longer exercised the scoped path. Scope them by the account's ID, which
is what a group stores now, and pin that a name entry contributes only
its literal value to the SQL scope.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(auth,api): bind account names to the identifier in scope checks

Review (rE7) found that an ID-scoped user could pass the purchase gate
for an account outside their scope. resolveAccountNamesByID keyed
names by both account ID and the free-text external ID in one map, so
an editor of in-scope account Z who set Z's external ID to victim Y's
ID got Z's name back for Y. AccountScope.Allows then resolved that
name to Z, which is in scope, and allowed a purchase in Y. The same
map fed the dashboard, ladder and RI reshape filters.

Decide those gates on the record's cloud_account_id itself, the ID
the purchase resolves its credentials from, and delete the dual-keyed
map. Bind the name path in Allows as well, so a name only identifies
an account through that account's own external ID and never through a
value that is some registered account's ID. Callers that pass a name
for an external ID already resolve it within the record's provider or
require exactly one owner.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/few Limited audience priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/feat New capability urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(plans): settle a failed-with-submission row so a managed plan can be deleted

2 participants