Skip to content

feat(ladder): stamp ladder_tranches.settled_at when a tranche completes - #849

Merged
cristim merged 2 commits into
mainfrom
fix/ladder-amend-overbuy-842
Oct 13, 2026
Merged

cristim merged 2 commits into
mainfrom
fix/ladder-amend-overbuy-842

Conversation

@cristim

@cristim cristim commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Refs #842 #843 #118

PR-A of the #842 plan. Inert: nothing calls the new helper yet and no production behavior changes except the extra column stamped on completion.

  • Migration 000117: nullable ladder_tranches.settled_at, backfilled with now() on pre-existing completed rows (never earlier than the real settle time, so sums err toward counting more), then CHECK (status <> 'completed' OR settled_at IS NOT NULL) enforced (a NOT VALID check still fires on the ON DELETE SET NULL updates and made legacy rows undeletable). Idempotent up, clean down.
  • TransitionLadderTrancheTx sets settled_at = clock_timestamp() in the same UPDATE when the new status is completed.
  • settledLadderCommit(ctx, q, configID, anchor): sum of the config's completed tranches with settled_at > anchor (strict); failed/canceled/fired/scheduled are not. PR-B (amend ceiling) and PR-C (fix(ladder): E read vs fired->completed hand-off race over-buys once executor lands #843) consume it.
  • LadderTrancheDB.SettledAt added to the insert and to the two list scanners.

Verification (local PG17, no mocks, LADDER_SETTLED_TEST_DATABASE_URL, db pr842a_*): new test covers stamp on completed only, lost CAS leaves the stamp, constraint rejects an unstamped completed insert/update, boundaries (equal to anchor, one microsecond after, failed, other config); migration test (CI integration job): backfill, unrelated update and deletes of account/config/execution with a legacy completed row, CHECK, idempotent re-run, down/up. Mutation (drop the stamp) fails it. The existing Ladder* integration tests were also run against the same PG17 and pass; three fixtures that insert completed rows now carry a stamp. The Docker-based tests skip locally and run in CI.

Out of scope: the amend ceiling (PR-B) and the E-vs-completed race (#843, PR-C).

🤖 Generated with Claude Code

Adds the nullable settled_at column (migration 000117), set to
clock_timestamp() in the same UPDATE that moves a tranche to completed
(TransitionLadderTrancheTx), a NOT VALID CHECK that every completed row
carries it, and settledLadderCommit: the sum of a config's completed
tranches settled strictly after an explicit anchor. Legacy completed rows
with a NULL stamp are counted whatever the anchor (fail closed); failed and
canceled rows are not.

Inert: nothing calls settledLadderCommit yet. PR-B (amend ceiling) and PR-C
(#843) consume it.

Refs #842 #843 #118

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim cristim added urgency/this-sprint Within the current sprint priority/p1 Next up; this sprint severity/high Significant harm impact/few Limited audience effort/m Days type/bug Defect triaged Item has been triaged labels Oct 13, 2026
@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 12 billable files and costs up to $3.00.

  • 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 26 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available. Your 59 included PR review attempts over the past 7 days set your current allowance at 2 reviews 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: 7ebf45ea-0b20-4288-a726-ed19dfb6881b
📥 Commits

Reviewing files that changed from the base of the PR and between 3c5a62b and 5c3c72f.

📒 Files selected for processing (12)
  • internal/api/handler_purchases_retry_ladder_integration_test.go
  • internal/config/store_postgres_ladder.go
  • internal/config/store_postgres_ladder_lifecycle.go
  • internal/config/store_postgres_ladder_lifecycle_integration_test.go
  • internal/config/store_postgres_ladder_reconcile.go
  • internal/config/store_postgres_ladder_settled.go
  • internal/config/store_postgres_ladder_settled_integration_test.go
  • internal/config/store_postgres_ladder_test.go
  • internal/config/types.go
  • internal/database/postgres/migrations/000117_ladder_tranches_settled_at.down.sql
  • internal/database/postgres/migrations/000117_ladder_tranches_settled_at.up.sql
  • internal/database/postgres/migrations/000117_ladder_tranches_settled_at_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

cristim commented Oct 13, 2026

Copy link
Copy Markdown
Member Author

Gate verdict (Opus gate agent): CHANGES REQUESTED at 873f24f

F1 (blocking, reproduced): the NOT VALID CHECK makes legacy completed rows block account, config and execution deletes

000117_ladder_tranches_settled_at.up.sql:21: NOT VALID only skips validating existing rows. Postgres still checks the constraint on every new row version, and that includes the UPDATE that an ON DELETE SET NULL foreign key runs. ladder_tranches has three such foreign keys (000081_ladder_tranches.up.sql:23,24,31: config_id, run_id, execution_id). So a pre-migration completed row with NULL settled_at (the state the PR says it supports) cannot have its parent deleted.

Reproduced on PG17.11 against the DB the PR's own test leaves behind. The down/up subtest makes every completed row a NULL legacy row:

  • DELETE FROM ladder_configs WHERE id = <cfg> fails with 23514 ladder_tranches_completed_has_settled_at, context UPDATE ONLY "public"."ladder_tranches" SET "config_id" = NULL
  • DELETE FROM cloud_accounts WHERE id = <acct> (the production path at internal/config/store_postgres.go:3810; it cascades to ladder_configs and then runs SET NULL on tranches) fails with the same 23514
  • an unrelated UPDATE ladder_tranches SET status_reason = 'x' on a legacy row also fails with 23514
  • DELETE FROM purchase_executions uses the same mechanism through execution_id. Two production paths run it: retention in cmd/cleanup-lambda/main.go:91 and internal/config/store_postgres.go:2151. Inferred from the same mechanism, not run separately.

This means the PR is not inert. It is latent today only because ReconcileLadderTranches has no production caller on main, so production probably has no completed rows. That same fact means the NULL-tolerant design buys nothing. Any later down/up rollback turns every completed row into a NULL legacy row and triggers the failure.

Suggested fix (author's call):

  1. In the up migration, backfill UPDATE ladder_tranches SET settled_at = clock_timestamp() WHERE status = 'completed' AND settled_at IS NULL. Then add the CHECK as valid, or VALIDATE CONSTRAINT it.
  2. The migration time is an upper bound on the true settle time, so settled_at > anchor counts a superset of the true set. That keeps the fail-closed behavior without NULLs.
  3. The settled_at IS NULL OR branch (store_postgres_ladder_settled.go:22) can then go or stay as a defense.
  4. Add a test: a completed row inserted before 000117, then migrate up, then delete its cloud account succeeds.

Non-blocking

  • F2: TestPostgresStoreDB_LadderSettledAt skips in CI because .github/workflows/ci.yml does not set LADDER_SETTLED_TEST_DATABASE_URL. The CHECK, the stamp and the predicate boundaries are verified only locally. Consider a testcontainers variant (setupLadderStore).
  • F3, surviving mutant: dropping the config scope (WHERE config_id = $1 changed to always true) passes the test. The assertion at store_postgres_ladder_settled_integration_test.go:130 is vacuous because no other-config row settles after now()+1h.

Verified OK

  • 000117 is the next free migration on main (116 is feat(api): require execute:purchases to arm ladder execution #847's). The migration is additive and idempotent, and down drops both the constraint and the column.
  • The stamp is clock_timestamp() inside the UPDATE and is applied only when the target status is completed. A lost CAS leaves it unchanged.
  • TransitionLadderTrancheTx is the only status writer that sets completed. The claim sets fired, and the sweep and amend never set completed.
  • The predicate uses a strict > with an explicit anchor, counts NULL legacy rows, never counts failed or cancelled rows, and filters on config_id (indexed).
  • The helper has no production caller.
  • Both LadderTrancheDB scanners were updated.
  • The boundary test truncates to microseconds, so it is safe on Linux.

Local evidence

Exact-head git archive, GOTOOLCHAIN=go1.26.9, PG17.11 throwaway cluster:

  • New test PASS, against a DB named with the pr842a_ prefix.
  • Unit tests for internal/{config,purchase,api,database}/... exit 0.
  • go vet -tags integration is clean.

Mutation run (9 mutants, one run): 8 killed (fixed-past stamp, dropped stamp, stamp on failed, dropped CHECK, > changed to >=, > changed to <, counting failed, dropping the NULL branch). 1 survived (config scope, F3). The Docker-based ladder integration tests were not run locally; CI runs them.

Not merged.

…leted rows stay deletable

A NOT VALID CHECK is still evaluated on every UPDATE of a row, including the
ON DELETE SET NULL updates of config_id/run_id/execution_id, so a legacy
completed row with NULL settled_at made deleting its account, config or
execution fail with 23514. Backfill completed rows with now() (never earlier
than the real settle time, so sums over settled_at > anchor err toward
counting more), then add the CHECK validated. Drop the now-unreachable NULL
branch from settledLadderCommit.

Adds a migration test (runs in the CI integration job) covering backfill,
unrelated updates and the three deletes on a legacy completed row, the CHECK,
idempotence and down/up. Moves the stamp/boundary test onto the shared
container helper so it runs in CI too, and asserts config scoping. Fixes the
retry-ladder test fixture that wrote completed without a stamp.

Refs #842 #843 #118

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

cristim commented Oct 13, 2026

Copy link
Copy Markdown
Member Author

Gate verdict (Opus gate agent): APPROVED at 5c3c72f

I re-gated the diff 873f24fd..5c3c72f5.

F1 is resolved. The migration now backfills settled_at = now() on pre-existing completed rows and adds the CHECK as validated (convalidated = t). The predicate no longer has a NULL branch.

  • The backfill value is justified. It is never earlier than the true settle time, so settled_at > anchor counts a superset of the true set. The sum can over-count, never under-count.
  • Reproduced on PG17.11 with the exact-head 000117 down and up files. I seeded a legacy completed tranche (NULL settled_at) linked to an account, a config and an execution, then applied up. All of these now succeed:
    • the row is backfilled
    • UPDATE status_reason
    • DELETE FROM purchase_executions, ladder_configs and cloud_accounts, with the SET NULL cascades leaving the row stamped
  • A re-run of up is a no-op (UPDATE 0, one constraint). An unstamped completed insert is still rejected with 23514.

F2 is resolved. CI run 38385096748 ran both TestPostgresStoreDB_LadderSettledAt (4 subtests) and TestMigration_LadderTranchesSettledAt through the testcontainers fallback. Both PASS; neither was skipped.

F3 is resolved. The dropped config_id filter mutant is now killed.

Other checks

  • 000117 is still the next free migration on main fc55fc7.
  • The PR merges cleanly onto main. Main's new commit adds no completed-tranche writers or fixtures.

Local evidence

Exact-head git archive, GOTOOLCHAIN=go1.26.9, PG17.11 throwaway cluster:

  • Settled integration test PASS.
  • Unit tests for internal/{config,purchase,api,database}/... exit 0.
  • go vet -tags integration is clean.
  • git-secrets with --register-aws is clean on the changed files, with both BSD and GNU grep.

Mutation run (8 mutants, one run)

7 killed:

  • stamp set to a fixed past time
  • stamp dropped
  • stamp also set on failed
  • CHECK dropped
  • > changed to >=
  • > changed to <
  • config scope dropped

1 survived (non-blocking): counting failed rows. It is equivalent in practice, because only the completed transition stamps settled_at, so failed rows always have NULL and never match settled_at > anchor.

CI is green and mergeState is CLEAN at this SHA.

@cristim
cristim merged commit 438523e into main Oct 13, 2026
25 checks passed
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/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant