feat(purchase): persist no_submission proof on failed executions (#118 PR-C1)

[cristim]

Refs #118

PR-C1 of the ladder tranche reconciler. Persisted proof only: C2 (the reconciler, tranche status_reason, chain walk) is not included and follows as a separate stacked PR. Nothing fires yet: no executor exists, ladder flags stay off, no tranche can reach fired, so this changes no purchase behavior.

Migration number is claimed at merge. 000113 is a placeholder (current max on main is 000112); renumber to max+1 right before merge.

What

• Migration 000113: purchase_executions.no_submission BOOLEAN NOT NULL DEFAULT FALSE (ADD COLUMN IF NOT EXISTS, additive; old code ignores it). Existing rows read false (no proof, fail closed).
• PurchaseExecution.NoSubmission; the upsert keeps the proof only for a failed write: no_submission = CASE WHEN $3 = 'failed' THEN purchase_executions.no_submission OR $28 ELSE false END. Among failed writes it is monotone (a stale failed copy cannot clear it); any non-failed write clears it, so a stale approved/completed upsert cannot leave the proof on a row that reaches the provider.
• All 12 SELECT/RETURNING column lists and scanExecutionRows read the column.
• Stamped true only where the code proves no cloud call was made: saveTerminalExecution on a beforeSubmissionError failure with no rec purchased (now also for direct executes, which previously carried no persisted trace), and stampFailedBarePlanStep (pending/notified bare step failed before the executor). NOT stamped: the reaper, safeFail, outcome-unknown failures, errAllAccountsFailed.
• 000100_..._test.go now migrates to head before using the store, since the store selects columns added after 000100.

Verification

Real PostgreSQL 17.11 (throwaway local instance; Docker unavailable here; temporary helper override reverted before commit). go test -tags integration passes for: new migration test (pre-existing row = false, up statement idempotent, down drops the column), TestNoSubmission_UpsertIsMonotone (false->true persists, stale false upsert does not clear), TestNoSubmission_ReadersReturnTheFlag (GetExecutionByID, GetExecutionByPlanAndDate, ListStuckExecutions, TransitionExecutionStatus RETURNING), and the full internal/config, internal/purchase, internal/api, internal/scheduler and migrations integration suites. internal/server integration was not completed locally (timed out at 600s); CI covers it.

• Gate fix (B1, 2fc96f8): the earlier plain OR kept the proof through the approve-path sequence (approvals.go:392 CAS -> claimAndExecute pre-submission failure -> stale approved upsert at :412 -> provider call at :421). Reproduced on real PG17 with the old SQL (3 new tests fail), pass with the CASE guard. Tests: TestNoSubmission_NonFailedWriteClearsProof (completed and approved), TestNoSubmission_StaleApproveUpsertDoesNotKeepProofOnSubmittedRow; the readers test now seeds the proof on a failed row.
• Mutation: upsert changed to a plain overwrite (no_submission = $28) fails the monotone test ("a stale false upsert must not clear the proof").
• Unit (labelled mock): TestSaveTerminalExecution_ReleasesSuppressionsOnlyBeforeSubmission extended with the proof expectation per case; TestStampFailedBarePlanStep_RecordsNoSubmissionProof; pgxmock column lists updated. go test ./internal/... 7390 passed; make lint 0 issues; git secrets --scan with --register-aws run under BSD and GNU grep.

Known residual race (do not trust the flag yet)

The other ordering of the same race is NOT closed here: if the stale approved save lands before the pre-submission failure, the final failed write still ORs onto a true proof. Root cause predates this PR: the approve path runs executeAndFinalize on an approved row without claiming running (approvals.go:421) while claimAndExecute accepts approved (manager.go:180), so two executors can run one row. C2 and B2 must not treat no_submission as proof until that race is closed (e.g. stamp the proof only through a CAS from running the executor owns). Follow-up issue to be filed.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 13 billable files and costs up to $3.25.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 40 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 60 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 526d2214-5c79-4ab6-aef0-f195f7626182

📥 Commits

Reviewing files that changed from the base of the PR and between 60f4d9b and 3bc2dfc.

📒 Files selected for processing (13)

• internal/config/store_postgres.go
• internal/config/store_postgres_no_submission_integration_test.go
• internal/config/store_postgres_pgxmock_test.go
• internal/config/store_postgres_ramp_step.go
• internal/config/types.go
• internal/database/postgres/migrations/000100_hash_approval_tokens_at_rest_test.go
• internal/database/postgres/migrations/000113_purchase_executions_no_submission.down.sql
• internal/database/postgres/migrations/000113_purchase_executions_no_submission.up.sql
• internal/database/postgres/migrations/000113_purchase_executions_no_submission_test.go
• internal/purchase/ladder_no_submission_test.go
• internal/purchase/manager.go
• internal/purchase/notifications.go
• internal/purchase/plan_step_atomic_test.go

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

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

[cristim]

Gate review (Opus, claude-opus-5-5): CHANGES REQUESTED at 15ab64b

Blocking

B1. internal/config/store_postgres.go:1077: the plain no_submission = purchase_executions.no_submission OR $28 lets a stale non-failed upsert keep the proof on a row that then reaches the provider. Plan amendment P2-2 asked for the CASE guard; the PR body doesn't mention that it was dropped.

Reachable trace at this head:

1. transitionApproveAndExecute CAS pending->approved (internal/purchase/approvals.go:392) and keeps updated in memory.
2. Cron/SQS claimAndExecute accepts approved (internal/purchase/manager.go:180). It claims the row, fails before submission (for example staleAzurePricingRefusal, manager.go:225-227) and saves failed + no_submission=true (manager.go:281-288).
3. The approve path's audit upsert of the stale updated (approvals.go:412) sets status back to approved. OR keeps the flag true.
4. approvals.go:421 runs executeAndFinalize(updated) without a running claim. The provider call is sent and the outcome is unknown, so the row is saved failed + false. OR keeps the flag true.

The row ends failed + no_submission=true even though a provider call was made. C2 would release that tranche from netting, which could lead to a double buy.

Reproduced on real PostgreSQL 17.11 with a store-level replay of steps 1-4 (gate-only test, not pushed):

after stale approve upsert: status=approved no_submission=true
final: status=failed no_submission=true
--- FAIL: TestGate828_StaleApproveUpsertKeepsFalseProofOnSubmittedRow
--- FAIL: TestGate828_CompletedWriteClearsProof   (P2-2's required test: failed+true, then completed+false => still true)

With no_submission = CASE WHEN EXCLUDED.status = 'failed' THEN purchase_executions.no_submission OR EXCLUDED.no_submission ELSE false END, both pass (status=approved no_submission=false, final ... no_submission=false), and TestNoSubmission_UpsertIsMonotone still passes.

Required:

• Apply the P2-2 CASE guard.
• Add P2-2's test (save failed+true, upsert completed+false => false) and a stale non-failed upsert test (failed+true, upsert approved+false => false).
• Note: TestNoSubmission_ReadersReturnTheFlag (store_postgres_no_submission_integration_test.go:51-54) seeds the proof on an approved row, which the guard correctly clears. Seed it on a failed row instead, for example by listing failed in ListStuckExecutions or reading via GetExecutionsByStatuses. Then assert the reader through a failed->(other) CAS, or drop that RETURNING assertion in favour of a failed row read.

Residual, non-blocking for C1 (follow-up issue; must be settled before C2 trusts the flag)

The guard does not close the other ordering of the same race: if step 3 lands before step 2, the final failed+false write still ORs onto the true from step 2. The root cause predates this PR. The approve path executes an approved row without claiming running (approvals.go:421), and claimAndExecute accepts approved (manager.go:180), so two executors can run the same row. C2 should not treat no_submission as proof until that race is closed. Alternatively, each executor should stamp the proof only through a CAS from running that it owns.

Checked and OK

• Stamping: only saveTerminalExecution on beforeSubmissionError with no rec purchased (manager.go:281-288; wrappers are at execution.go:62,65,72,99,103,202 and manager.go:227, and all of them run before any provider call) and stampFailedBarePlanStep (notifications.go:286, reached via the pending/notified->failed CAS at :263 and the fresh-row path at :230). Not stamped: safeFail (manager.go:577), reaper (reaper.go:175/229), errAllAccountsFailed, armed-redrive refusal.
• Readers: all 12 column lists (store_postgres.go:1210,1406,1518,1543,1585,1606,1630,1678,1708,1735,1944; store_postgres_ramp_step.go:331) plus scanExecutionRows read the column. The other SELECTs are counts or ids only.
• Migration 000113: expand-only, ADD COLUMN IF NOT EXISTS, constant default, down uses IF EXISTS, test present. 000113 is still free on main (tip 000112), and the head contains main.
• 000100 test change: migrating to head before using the current store is correct and needed.

Local evidence at this head (git archive 15ab64b, GOTOOLCHAIN=go1.26.9, throwaway PostgreSQL 17.11 on 127.0.0.1:54419, temporary local test-helper override, not committed)

go test -tags integration -count=1: internal/config ok (69.8s), internal/purchase ok (13.7s), internal/database/postgres/migrations ok (43.6s; the first run failed only because the harness was missing the hardcoded cudly_test DB, and it passed once that DB was created). The PR's own tests pass. The two gate repro tests above fail at this head.

Verdict: CHANGES REQUESTED (B1). Not merged.

CI at this head: Unit Tests failed in "Download dependencies" (proxy.golang.org stream INTERNAL_ERROR for klauspost/compress, an infra flake, not code). Not rerun, since the head has to change anyway.

[cristim]

Gate verdict (Opus, claude-opus-5-5): APPROVED at 3bc2dfc

Re-gated after the B1 fix (2fc96f8), then gh pr update-branch merged main (#827) in (3bc2dfc). The PR's own diff against main is identical before and after the update. 000113 is still free (main tip 000112).

• B1 is resolved: the upsert is now CASE WHEN $3 = 'failed' THEN purchase_executions.no_submission OR $28 ELSE false END ($3 is the row status). I re-ran my stale-approve replay against the new code: after stale approve upsert: status=approved no_submission=false, final: status=failed no_submission=false, PASS. P2-2's completed-write test passes.
• Non-vacuity: reverting to a plain OR $28 makes the author's TestNoSubmission_NonFailedWriteClearsProof/{completed,approved} and TestNoSubmission_StaleApproveUpsertDoesNotKeepProofOnSubmittedRow fail.
• Merged feat(purchase): make notified real and pin the scheduler claim to the gated snapshot #827: its new failClaimedBeforeSubmission (step_cap.go:63) runs the step-cap recheck before any provider call and saves through saveTerminalExecution with beforeSubmission, so stamping there is correct. Its new pinned transition keeps the no_submission RETURNING column (12/12 column lists).
• Local, exact head (git archive 3bc2dfc, GOTOOLCHAIN=go1.26.9, throwaway PostgreSQL 17.11 on 127.0.0.1:54419, local-only test-helper override): go test -tags integration passes for internal/config (51.7s), internal/purchase (9.4s) and internal/database/postgres/migrations (48.3s). Unit tests pass for config and purchase.
• CI: 26/26 pass, mergeStateStatus CLEAN.
• Residual race (approve path vs claimAndExecute running the same row) is tracked in fix(purchase): approve and scheduled-fire paths execute approved rows without claiming them as running #829 and must be closed before C2 trusts the flag.