Skip to content

fix(ladder): refuse to execute an execution whose tranche is already settled - #853

Merged
cristim merged 2 commits into
mainfrom
feat/ladder-gate-settled-tranche
Oct 13, 2026
Merged

cristim merged 2 commits into
mainfrom
feat/ladder-gate-settled-tranche

Conversation

@cristim

@cristim cristim commented Oct 13, 2026

Copy link
Copy Markdown
Member

Refs #118

Second review finding on the operator resolve plan (#848 hole): the execute-time recheck (ladderGateRefusal) read the ladder config switches and the run status but never the tranche's own status. An execution carrying a tranche key but not on the locked retry chain could still buy after the tranche was failed, completed or canceled (for example after an operator resolve, PR5b). This lands first so PR5b does not depend on it being right.

  • GetLadderExecutionGate also returns t.status as TrancheStatus.
  • ladderGateRefusal refuses when the tranche is settled (LadderTrancheSettled: anything but scheduled or fired) or the status is missing. The refusal is a beforeSubmission error, so the row ends failed with no_submission stamped, as for the other gate refusals.
  • Non-ladder keys still return no gate and are untouched.

Verification (local macOS, real PG17 for the SQL; the rest is the existing mock-store harness, labelled FIXTURE in the test file): TestLadderRecheck_* unit tests pass, including new cases for failed, completed and cancelled tranches and a missing status, and a new positive control that fired and scheduled tranches still buy exactly once (an inverted check fails it). TestGetLadderExecutionGate (integration, PG17) asserts the real tranche status. Mutants run: check removed, check inverted; both fail the suite. internal/purchase integration suite passes.

🤖 Generated with Claude Code

…settled

The execute-time recheck read the config switches and the run status but never the tranche status. An execution that carries a tranche key but is not on the locked retry chain could still buy after the tranche was failed, completed or canceled. GetLadderExecutionGate now returns the tranche status and ladderGateRefusal refuses unless the tranche is scheduled or fired; a missing status refuses too. The refusal is a beforeSubmission error as before.

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 4 billable files and costs up to $1.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 12 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: cf61029c-1868-4bed-95ee-30b2a6b701a2
📥 Commits

Reviewing files that changed from the base of the PR and between 0e31110 and d8338bc.

📒 Files selected for processing (4)
  • internal/config/store_postgres_ladder_gate_integration_test.go
  • internal/config/store_postgres_ladder_reconcile.go
  • internal/purchase/ladder_execution_recheck.go
  • internal/purchase/ladder_execution_recheck_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 (Opus, money path) at 9db3305: CHANGES REQUESTED, not merged.

Blocker

  • CI Lint Code fails (run 38400037084, job 115304995347): internal/purchase/ladder_execution_recheck_test.go:90:13: "cancelled" is a misspelling of "canceled" (misspell). Line 90 is the new "tranche cancelled" case (both the case name and the expected substring "its tranche is cancelled"). Fix per repo convention, e.g. a trailing //nolint:misspell // "cancelled" is the stored ladder status value as in internal/api/handler_purchases_retry_ladder_integration_test.go:50, or build the strings from ladder.TrancheStatusCancelled.

Review of the logic (no defects found; reviewed by reading the archived head)

  • Refuses only settled tranches: config.LadderTrancheSettled (store_postgres_ladder_reconcile.go:182-184) is false only for scheduled and fired, so the claim path (store_postgres_ladder_lifecycle.go:127-133, which sets fired, and the in-tx create callback that still sees scheduled from other connections) is not refused. Unknown strings and "canceled" (not a tranche enum value; pkg/ladder/store.go:61-65 has only scheduled/fired/completed/cancelled/failed) count as settled, i.e. fail closed. Migration 000081:30 makes status NOT NULL with no CHECK, so the nil branch is unreachable on a found row but harmless.
  • No-match still returns (nil, nil) (text compare unchanged); DB error still refuses (ladder_execution_recheck.go:33-35).
  • Refusal goes through preExecutionRefusal -> beforeSubmission (manager.go:279-290), the single funnel used by approvals.go:425, scheduled_fire.go:100, manager.go:214 (claim/SQS) and :580 (re-drive). The reconciler transitions only fired tranches (ladder_reconcile.go:138, edge table lifecycle.go:33-34), so a refused stray execution on a settled tranche cannot reopen or alter it; Retry already 409s on settled tranches (handler_purchases.go:2292).

Non-blocking follow-ups

  • No unit case for an unknown or "canceled" status string: a mutant replacing LadderTrancheSettled with an explicit failed/completed/cancelled denylist would survive. Suggest one case with TrancheStatus "canceled" or "expired" expecting refusal.

Local evidence: none run this pass. The machine has ~119 MB free (ENOSPC hit during the gate), so no PG17 cluster or Go test build was possible; CI is red on lint regardless, and the next gate at the fixed head must run the unit + integration suites locally.

Refs #118

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

cristim commented Oct 13, 2026

Copy link
Copy Markdown
Member Author

Gate (Opus, money path) at d8338bc: APPROVE, merging.

Delta since 9db3305 (previous gate) is test-only: //nolint:misspell on the cancelled case and a new "bogus" unknown-status case (ladder_execution_recheck_test.go:90-91). Production code unchanged from the logic reviewed earlier (no defects: refuses only non scheduled/fired tranches, unknown statuses fail closed, no-match stays "not ladder", DB error refuses, refusal is beforeSubmission through the single executeAndFinalize funnel, reconciler only moves fired tranches so a refused stray cannot reopen a settled one).

Local evidence (macOS, git archive of the exact head, GOTOOLCHAIN=go1.26.9, throwaway local PG17 instead of Docker via a test-helper override in the archive only):

  • go build ./..., go vet (plain and -tags integration) for config/purchase/api: ok.
  • Unit: config, purchase, api ok. TestLadderRecheck_*: all subtests PASS incl. failed/completed/cancelled/bogus/nil refused with zero provider calls, fired and scheduled buy exactly once, non-ladder keys unaffected, all 4 executor entries refuse.
  • Integration (-tags integration, real PG17): config, purchase, api packages ok; targeted Retry/Ladder/Reconcile/TestGetLadderExecutionGate run: 36 PASS, 0 FAIL. TestGetLadderExecutionGate ran (not skipped).
  • Extra throwaway real-PG probe (not committed): tranche moved fired->failed via TransitionLadderTranche, GetLadderExecutionGate returns "failed", LadderTrancheSettled true: PASS.
  • Mutants (one run): drop status check, invert, only-failed, denylist failed/completed/cancelled, drop nil check, swallow DB error: all KILLED.
  • git-secrets --register-aws on the 4 PR files: BSD exit 0, GNU exit 0.
  • CI: 26/26 checks pass on d8338bc; mergeStateStatus CLEAN.

Non-blocking follow-up (surviving mutant): replacing t.status in the GetLadderExecutionGate SQL with the constant 'fired' survives, because TestGetLadderExecutionGate only asserts a fired tranche. One integration subtest that settles the tranche and asserts the returned status would close it (my probe above shows the real code is correct).

@cristim
cristim merged commit 1932893 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