Skip to content

feat(ladder): reconcile fired tranches from execution outcomes (#118 PR-C2) - #837

Merged
cristim merged 1 commit into
mainfrom
feat/ladder-reconciler-c2-118
Oct 13, 2026
Merged

cristim merged 1 commit into
mainfrom
feat/ladder-reconciler-c2-118

Conversation

@cristim

@cristim cristim commented Oct 13, 2026

Copy link
Copy Markdown
Member

Refs #118

PR-C2 of the ladder tranche reconciler, stacked on the merged C1 (#828) and the executor race fixes (#831, #833; #829 closed). Nothing calls the reconciler yet: no scheduled task, no executor, ladder flags stay off, so no tranche can reach fired in a running system. The scheduled-task wiring, the operator resolve action and the production commitment-visibility adapter stay deferred to B2.

Migration number is claimed at merge. 000114 is the next free number on main today (tip 000113); renumber to max+1 right before merge.

What

  • Migration 000114: ladder_tranches.status_reason TEXT (nullable, ADD COLUMN IF NOT EXISTS). The overdue sweep now stamps expired.
  • TransitionLadderTranche(..., reason) plus a Tx variant; a failed or canceled transition without a reason is an error.
  • ListFiredLadderTranches and LockLadderTrancheChainTx: lock the tranche row, follow retry_execution_id from the tranche's execution (depth cap 20; a cycle hits the cap and reads as unresolved), then lock and re-read the tail execution row with FOR UPDATE. LinkRetryExecutionAtomic updates that same row, so a Retry cannot link a successor mid-decision.
  • purchase.ReconcileLadderTranches(store, visibility, now) (package function over a narrow store interface, so no mock or interface churn):
    • failed only if every node in the chain is failed with no_submission=true, or the tail is canceled/cancelled/expired and every earlier node is proven (status_reason = no_submission / approval_canceled / approval_expired);
    • completed only if the tail is completed and CommitmentVisible says so (an error or false keeps it fired);
    • partially_completed, a failure without proof (reaper, outcome unknown), pending/running executions and an unresolved chain stay fired and keep netting;
    • unresolved for more than 6h (LadderFiredStaleAfter, constant) is logged as an error and counted; the tranche is never changed;
    • the proof flag is read only from rows whose status is failed (the feat(purchase): persist no_submission proof on failed executions (#118 PR-C1) #828 upsert CASE guard clears it otherwise).

Re-verified on current main: no path leaves no_submission=true on a row that reached the cloud

Writers of the flag: saveTerminalExecution (only on a beforeSubmissionError with no rec purchased) and stampFailedBarePlanStep (pending/notified bare step). Every save goes through SavePurchaseExecutionTx, whose upsert keeps the flag only for a failed write and clears it for any other status; since #829/#833 terminal saves are status-guarded on running (SaveExecutionIfStatus), so the stale approve-path save can no longer resurrect a failed row. RecoverReapedExecutionTx writes completed/partially_completed, which clears it. Reaper, safeFail and errAllAccountsFailed never stamp. The reconciler also requires proof on every earlier chain node, so a reaped first attempt keeps the tranche fired.

Open risk carried to B2 (blocking there, not here)

A Retry that links after the reconciler commits failed would give a failed tranche a live approvable successor. B2 must make Retry refuse an execution whose ladder tranche is terminal (plan amendment A2(b)).
Also: the visibility check runs while the reconciler holds the tranche and tail row locks; the B2 adapter must be fast or cached.

Verification

Real PostgreSQL 17.11 (throwaway local instance; Docker unavailable here; temporary helper override reverted before commit). go test -tags integration passes for the full internal/config, internal/purchase, internal/api and migrations suites. New real-PG tests: every mapping incl. both canceled spellings, proof flag ignored on a running row, idempotent rerun, stale alert count, cycle and over-depth chains stay fired, a 3-node chain whose first node was reaped stays fired (and fails once that node is proven), migration 000114 (existing row NULL, up idempotent, down drops), reason stored and required, sweep stamps expired; in internal/api: the real persistRetrySuccessorTx blocks while the reconciler holds the tail row (and a Retry that linked first keeps the tranche fired).

  • Mutants (capped, one run): tail not locked (the real-Retry test hangs/fails), predecessors ignored (2 tests fail), proof flag read on any status (2 tests fail) are all caught. Two survivors: visibility error treated as visible is an equivalent mutant with this stub; the visited-set cycle guard was dead code under the depth cap, so it was deleted.
  • Labelled fixture: commitment visibility is a stub (FIXTURE); the pure mapping table is a unit test (no DB).
  • make lint 0 issues, go test ./internal/... passes, git secrets --scan with --register-aws under BSD and GNU grep.
  • Size: about 305 non-test lines, the rest tests (the 400-line target is exceeded by tests only; this is already the second half of the plan's split).

🤖 Generated with Claude Code

…PR-C2)

Add the tranche reconciler and the store pieces it needs. Nothing calls it yet
(no scheduled task, no executor, flags off), so no tranche can reach fired in
a running system.

- migration 000114: ladder_tranches.status_reason (additive); the overdue sweep
  stamps 'expired'; TransitionLadderTranche takes a reason (required for failed
  and canceled) and gains a Tx variant.
- ListFiredLadderTranches and LockLadderTrancheChainTx: lock the tranche row,
  follow retry_execution_id (depth cap 20, which also stops a cycle), then lock
  and re-read the tail execution row, the same row LinkRetryExecutionAtomic
  updates.
- ReconcileLadderTranches: failed only when every node of the chain is failed
  with no_submission (or the tail is canceled/expired with proven
  predecessors); completed only when the commitment is visible; partially
  completed, unproven failures and unresolved chains stay fired and keep
  netting; unresolved over 6h is logged as an error. The proof is read only
  from failed rows.

Refs #118

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim cristim added urgency/this-sprint Within the current sprint triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm impact/few Limited audience effort/m Days type/bug Defect 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 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: 0879a0b2-ce5b-443b-a0a8-e60699a0490b
📥 Commits

Reviewing files that changed from the base of the PR and between f1ae0d8 and 452115a.

📒 Files selected for processing (12)
  • internal/api/handler_ladder_reconcile_retry_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_lifecycle_test.go
  • internal/config/store_postgres_ladder_reconcile.go
  • internal/database/postgres/migrations/000114_ladder_tranches_status_reason.down.sql
  • internal/database/postgres/migrations/000114_ladder_tranches_status_reason.up.sql
  • internal/database/postgres/migrations/000114_ladder_tranches_status_reason_test.go
  • internal/purchase/ladder_reconcile.go
  • internal/purchase/ladder_reconcile_integration_test.go
  • internal/purchase/ladder_reconcile_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: APPROVE at 452115a (independent adversarial review, Opus 5.5 claude-opus-5-5, money path)

Review (code read at the exact head)

  • Failed only with proof on the whole chain: internal/purchase/ladder_reconcile.go:78-99. Tail failed needs NoSubmission; tail canceled/cancelled/expired and failed both go through failedIfPredecessorsProven, which requires every earlier node to be failed with no_submission=true. Reaped, safeFail, errAllAccountsFailed and unknown outcomes keep the tranche fired. The flag is only read on a failed node (the tail branch is inside case StatusFailed; predecessors check status first).
  • Canceled/expired are pre-claim only: every writer CAS-es from pending/notified/paused (handler_purchases.go:521, store_postgres.go:1294-1325), scheduled (CancelScheduledExecutionAtomic, store_postgres.go:1373-1383; the claim moves scheduled to running before any provider call since fix(purchase): claim running before any provider call, guard terminal saves #833) or pending/notified (store_postgres_ramp_step.go:370).
  • Retry race: LockLadderTrancheChainTx (internal/config/store_postgres_ladder_reconcile.go:72-97) locks the tranche FOR UPDATE, walks at most 20 nodes, then re-reads the tail FOR UPDATE; a tail that gained a successor, or a chain cut by the cap (a cycle hits it), ends with RetryExecutionID != nil and stays fired (ladder_reconcile.go:63-65). LinkRetryExecutionAtomic (store_postgres.go:1159-1172) updates that same row, so the real persistRetrySuccessorTx blocks until the reconciler commits. The transition runs in the same tx (ladder_reconcile.go:126-140). The post-commit Retry case is carried to B2 as stated.
  • Completed only for status=completed with CommitmentVisible true; error or false stays fired. partially_completed stays fired. Stale alert at the LadderFiredStaleAfter = 6h constant, never changes the tranche.
  • Migration 000114: next free number on main now (main tip 7a0dff49 has 000113 as the max; no open PR claims 000114), ADD COLUMN IF NOT EXISTS / DROP COLUMN IF EXISTS, nullable, expand-only, with a migration test.
  • TransitionLadderTranche signature: only callers are tests in internal/config; it is not on StoreInterface, so no mock churn.
  • Not wired: ReconcileLadderTranches, LockLadderTrancheChainTx, ListFiredLadderTranches have no production caller; no flag, scheduler, handler or Terraform change in the diff.
  • Linux: CI Integration Tests (run 38330414726, ubuntu) at this SHA ran and passed all 8 new integration tests. Ages in the tests are relative to PG NOW() with a margin of 1h against a 6h threshold, and no test compares Go and PG timestamps for equality, so ns vs us precision does not apply.

Local verification (throwaway local PostgreSQL 17.11 cluster on 127.0.0.1, git archive of the exact head, GOTOOLCHAIN=go1.26.9, under the suite lock; testhelpers container helper overridden to the local cluster in the scratch copy only, never committed). Rows are fixtures; commitment visibility is a stub.

  • go build ./..., go vet (config/purchase/api/database): OK. go test config, purchase, api, database: ok.
  • go test -tags integration: internal/config ok (162s), internal/purchase ok (63s), internal/api ok (133s), internal/database/postgres/migrations ok (40s). All new tests PASS verbose.
  • Merged tree (current main 7a0dff49 + this PR, via git merge-tree): build, unit config/purchase/api, integration purchase and ladder/reconcile tests in api/config: ok.
  • Mutants (17 on the changed lines, one run). Killed 13, including the ones that matter: drop the every-node proof (integration 3-node reaped-first chain + unit fail), drop the tail FOR UPDATE (the real-Retry test fails; it hangs to the test timeout), partially_completed treated as completed, drop the depth cap (cycle test times out), ignore a truncated tail, failed without proof treated as failed, visibility ignored, visibility error treated as completed, expired stays fired, stale threshold, reason not required, reason not stored, sweep reason dropped.
  • git-secrets --register-aws --scan of the PR files with BSD grep and GNU grep: exit 0 both (an AKIA canary is caught by both).
  • CI at this SHA: all checks pass, mergeStateStatus CLEAN.

Non-blocking follow-ups

  • Surviving mutant: making a canceled tail skip the predecessor proof (ladder_reconcile.go:84) passes every test. The code is correct, but nothing pins "reaped E1, retry E2 canceled stays fired". Add that case to TestDecideFiredTranche and the integration chain test before B2 wires the reconciler.
  • Surviving mutant: dropping the tail re-read after the lock (store_postgres_ladder_reconcile.go:92-96) passes; the walk-to-lock window is not tested.
  • Equivalent mutants: predecessor status check removed (the feat(purchase): persist no_submission proof on failed executions (#118 PR-C1) #828 upsert CASE already clears the flag on non-failed rows); tranche lock without the status='fired' filter (the later CAS from fired catches it).
  • TestLadderReconcile_TailLockBlocksRealRetryLink (internal/api/handler_ladder_reconcile_retry_integration_test.go:125-129): on failure t.Fatalf leaves paused.release open, so the test hangs until the package timeout instead of failing fast with its message. Close release in t.Cleanup.
  • ListFiredLadderTranches takes the 200 oldest fired tranches (ladder_reconcile.go:20, store_postgres_ladder_reconcile.go:39-45). If more than 200 stay unresolved, newer ones are never examined. That is safe (they stay fired and keep netting) but they would never settle; B2 should page or skip past unresolved rows.
  • status_reason is not in any LadderTrancheDB scan yet (no reader needs it).

@cristim
cristim merged commit 7657a38 into main Oct 13, 2026
26 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