Repository navigation
feat(ladder): stamp ladder_tranches.settled_at when a tranche completes - #849
Conversation
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>
|
Warning Review limit reached
This review includes 12 billable files and costs up to $3.00.
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. View limit details
Comment |
Gate verdict (Opus gate agent): CHANGES REQUESTED at 873f24fF1 (blocking, reproduced): the NOT VALID CHECK makes legacy completed rows block account, config and execution deletes
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:
This means the PR is not inert. It is latent today only because Suggested fix (author's call):
Non-blocking
Verified OK
Local evidenceExact-head
Mutation run (9 mutants, one run): 8 killed (fixed-past stamp, dropped stamp, stamp on failed, dropped CHECK, 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>
Gate verdict (Opus gate agent): APPROVED at 5c3c72fI re-gated the diff F1 is resolved. The migration now backfills
F2 is resolved. CI run 38385096748 ran both F3 is resolved. The dropped config_id filter mutant is now killed. Other checks
Local evidenceExact-head
Mutation run (8 mutants, one run)7 killed:
1 survived (non-blocking): counting failed rows. It is equivalent in practice, because only the completed transition stamps CI is green and mergeState is CLEAN at this SHA. |
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.
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), thenCHECK (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.TransitionLadderTrancheTxsetssettled_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 withsettled_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.SettledAtadded to the insert and to the two list scanners.Verification (local PG17, no mocks,
LADDER_SETTLED_TEST_DATABASE_URL, dbpr842a_*): 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 existingLadder*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