feat(ladder): add ladder_run scheduled task (plan-only milestone) - #1362
Merged
Conversation
Implements the ladder_run scheduled task that runs the commitment-laddering planning engine on schedule and persists results without executing purchases. Changes: - config/types.go: add LadderRunDB and LadderTrancheDB types (nullable monetary snapshot fields as *float64, typed enums for status/layer/term/payment) - config/interfaces.go: widen StoreInterface with SaveLadderRun, GetLadderRun, SaveLadderTranches, LatestLadderRunStartedAt, TransitionLadderRunStatus - config/store_postgres_ladder.go: implement all five store methods with scanLadderRun/scanLadderTranche helpers (CAS UPDATE for TransitionLadderRunStatus) - mocks/stores.go: extend MockConfigStore with the five new ladder run methods (isExpected pattern, t.Cleanup AssertExpectations-compatible) - providers/aws/ladder/factory.go: NewFromAWSConfig factory + no-op stubs for read-side adapters not yet wired (on-demand series returns error, others empty) - server/app.go: add LadderCapabilityFactory field + wire to awsladder.NewFromAWSConfig in production NewApplication - server/handler.go: add TaskLadderRun constant, scheduledEventActions entry, dispatchTask entry - server/handler_ladder.go: handleLadderRun (global kill-switch, cadence gate, single-account only), executeLadderRun (Allocate+BuildTranches, plan JSON serialization, SaveLadderRun+SaveLadderTranches persistence) - server/handler_ladder_test.go: 21 unit tests covering global disable, config disable, healthy run, baseline error, cadence gate, config conversion errors, ratToFloat64Ptr, plan JSON shape; assert absence of PurchaseLayer/ReshapeBuffer - test_helpers_test.go + analytics/collector_test.go: extend inline mocks with the five new StoreInterface methods to satisfy compile-time checks Plan-only: no PurchaseLayer, ReshapeBuffer, approval tokens, or emails.
…atus) Resolves the FIX-THEN-SHIP findings on the ladder_run plan-only milestone. Blockers: - T10: add internal/config/store_postgres_ladder_test.go (integration-tagged, real PostgreSQL container). Covers SaveLadderRun NULL-monetary round-trip preserved as nil, LatestLadderRunStartedAt nil/MAX, SaveLadderTranches batch, and TransitionLadderRunStatus CAS win/lose. Verified locally: 13 subtests pass. - Fix the nil-baseline test trap: rename the baseline-ERROR test to TestExecuteLadderRun_BaselineError_NoRunPersisted and assert nothing is persisted; add TestExecuteLadderRun_NilBaseline_HoldPlan (nil low-water, no error -> status=planned, NULL target/gap, Hold-only), _ZeroGap_HoldPlan, and _MaxActionsExceeded (Allocate errors -> Errored, no persistence). Un-skip and implement TestHandleLadderRun_MultiConfigIsolation against the extracted runLadderConfigs loop (one healthy + one erroring config, isolation asserted). - buildTrancheDBRows now fails loud on a malformed AmountUSDPerHour instead of persisting a silently 0-coerced $0 tranche; the error propagates so the config is counted Errored with nothing partial written. Regression test added. Correctness: - assembleLadderPlan derives the monetary snapshot consistently with the authoritative allocation: target = baseline low-water * TargetCoveragePct/100 and gap = sum of planned allocation gaps (equals total_hourly_commit), instead of raw low-water and an independently recomputed target - existing. nil-safe: target/gap stay nil when the baseline is unavailable. Low/nit: - Correct the factory.go comments: a GetUsageBaseline failure is NOT persisted (executeLadderRun returns before SaveLadderRun); the config is only counted Errored. - TransitionLadderRunStatus is now typed (ladder.RunStatus) across the StoreInterface, PostgresStore impl, and all mocks; strings are confined to the DB boundary conversion. - Replace the bare "aws-ce" literal with the dataSourceAWSCostExplorer constant. - Strengthen TestExecuteLadderRun_HealthyRun to assert persisted tranches, purchase actions with term/payment, and the derived target/gap values. executeLadderRun split into persistLadderRun to stay within the cyclomatic limit after the added error branch. Gates: go build/vet ./... = 0; pkg go test ./ladder/... = 246 pass; internal/server + internal/config go test = 1031 pass; integration = 13 pass; gocyclo -over 10 (touched files) = clean.
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a scheduled, plan-only ladder run workflow with cadence and account gating, AWS capability wiring, PostgreSQL persistence for runs and tranches, CAS status transitions, and comprehensive unit and integration tests. ChangesLadder run execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ScheduledTask
participant handleLadderRun
participant LadderCapability
participant StoreInterface
ScheduledTask->>handleLadderRun: Dispatch ladder_run action
handleLadderRun->>StoreInterface: Load configuration
handleLadderRun->>LadderCapability: Fetch baseline and layer states
handleLadderRun->>LadderCapability: Allocate and build tranches
handleLadderRun->>StoreInterface: Save ladder run and tranches
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
… cadence) Addresses the gap-analysis findings on PR #1362's own diff before merge. B1 [HIGH] STS account resolution was a silent money-path fallback. handleLadderRun used resolveAccountID, which returns the "unknown" sentinel on STS failure (its contract is audit-only, not for scoping). A transient STS error therefore made every config get skipped as multi_account and handleLadderRun returned success - a silent no-op that EventBridge sees as a healthy run. Fix: resolve the caller account STRICTLY at the ladder call site via a new injectable Application.LadderAccountResolver (defaults to defaultLadderAccountResolver: LoadDefaultConfig + STS GetCallerIdentity, erroring on failure/empty). An unresolved account now aborts the whole task loud, persisting/recording nothing. resolveAccountID's contract in handler_ri_exchange.go is unchanged. B2 [MEDIUM] Non-transactional run+tranche persist. SaveLadderRun committed before SaveLadderTranches in separate transactions; a tranche-insert failure left a status=planned run with no tranches, and the 20h cadence gate then suppressed the retry for a full window. Fix: add SaveLadderRunWithTranches(ctx, run, tranches) to the store, inserting the run and its tranches in ONE WithTx transaction (shared insert SQL/args extracted; reuses scanLadderRun). Widened StoreInterface and extended every implementer (MockConfigStore + the three inline test mocks). The handler now persists via the atomic method. Monetary snapshot stays nullable (NULL != $0). B4 [LOW] Cadence gate failed OPEN on DB error. ladderWithinCadenceWindow swallowed a LatestLadderRunStartedAt error and proceeded, risking silent double-runs. Fix: propagate the error (now returns (bool, string, error)); processOneLadderConfig fails CLOSED, counting the config Errored (visible in LadderRunResult.Errored) instead of running it. Tests: TestHandleLadderRun_AccountResolutionFailure (B1: error + nothing persisted); SaveLadderRunWithTranches_Atomicity integration test (B2: success persists both, duplicate-tranche-ID rolls the run row back); TestLadderWithinCadenceWindow_DBError_FailsClosed + TestProcessOneLadderConfig_CadenceDBError_Errored (B4). Existing cadence tests updated to the 3-return signature; the disabled-config handler test injects a stub resolver. Gates: go build/vet ./... = 0; pkg go test ./ladder/... = 246 pass; internal/server + internal/config go test = 1034 pass; integration = 744 pass; gocyclo -over 10 (touched files) = clean.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/server/handler_ladder.go`:
- Around line 136-162: The ladder account resolver must reject an empty AWS
region before returning. In defaultLadderAccountResolver, validate awsCfg.Region
after loading the AWS config and return a descriptive error when it is empty;
preserve the existing STS account validation and successful return for
configured regions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6e1a8c8-797d-4366-b595-fc84e21c13b7
📒 Files selected for processing (9)
internal/analytics/collector_test.gointernal/config/interfaces.gointernal/config/store_postgres_ladder.gointernal/config/store_postgres_ladder_test.gointernal/mocks/stores.gointernal/server/app.gointernal/server/handler_ladder.gointernal/server/handler_ladder_test.gointernal/server/test_helpers_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/config/interfaces.go
- internal/analytics/collector_test.go
- internal/server/test_helpers_test.go
- internal/config/store_postgres_ladder_test.go
- internal/config/store_postgres_ladder.go
CodeRabbit Major: defaultLadderAccountResolver validated the STS account ID but not awsCfg.Region. An empty region passes resolution, then NewFromAWSConfig rejects every config, so the task returns all-Errored but still nil (success) - the same silent-degradation class as the B1 STS fix. Fix: validate the region up front and return a descriptive error when empty, aborting handleLadderRun before any config is processed. Extracted resolveLadderIdentity(ctx, awsCfg) from defaultLadderAccountResolver so the region short-circuit is unit-testable without loading real credentials or calling STS; the account-ID validation and normal successful return are preserved. Tests: TestResolveLadderIdentity_EmptyRegion_FailsLoud (empty region -> error before STS) and TestHandleLadderRun_EmptyRegionResolution_FailsLoud (handleLadderRun returns an error, result==nil, nothing persisted). Gates: go build/vet ./... = 0; internal/server + internal/config go test = 1036 pass; gocyclo -over 10 (handler_ladder.go) = clean.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
…on ladder ticks Address CodeRabbit findings on the L12 EventBridge ticks: - Tighten the two new Fargate EventBridge->ECS roles (ladder_run, fire_scheduled_purchases). Add ArnEquals ecs:cluster = aws_ecs_cluster.main.arn on the ecs:RunTask statement and StringEquals iam:PassedToService = "ecs-tasks.amazonaws.com" on the iam:PassRole statement, matching AWS best practice for EventBridge-triggered ECS tasks. Only the two new blocks are touched; the existing ri_exchange block is unchanged. - Document the intentional dispatch state above the ladder_run rule in both lambda/main.tf and fargate/main.tf: the "ladder_run" scheduled action is registered by the ladder_run task (PR #1362); the rule is default-off (count-gated) and enabling it before that task ships causes a loud dispatch error, never a silent no-op. No handler.go change here (that is #1362 scope; this PR stays terraform-only + default-off).
Member
Author
|
Merged to main as 7adacd7 after a final pre-merge adversarial review (verdict MERGE): all gates re-verified independently (build/vet 0, unit 1036, pkg/ladder 246, integration vs real Postgres green, gocyclo clean), branch was 0 commits behind main so the merge is conflict-free by construction, and every prior review finding (STS fail-loud, empty-region fail-loud, single-tx persist with rollback proof, cadence fails-closed, nil-baseline NULL columns, plan-only invariant) was re-verified against the committed code. Non-blocking follow-ups (positive skipped:multi_account test, two comment/naming nits) are being filed as a separate issue. Next in the chain: L2 wires the real Cost Explorer read adapters into providers/aws/ladder/factory.go so scheduled runs produce real plans. |
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
PR #1362 merged to main with factory.go's noopOnDemandSeriesSource returning []float64, while this branch changed the onDemandSeriesSource interface to return []DailyPoint (Date-carrying points for the stale-series freshness check). Merging without this alignment would break main despite no textual conflict (semantic conflict). Update the noop's return type to []DailyPoint. Behavior is unchanged: it still fails loud with the same not-yet-wired error, so plan runs against the unwired factory continue to be counted Errored.
This was referenced Jul 16, 2026
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
…contract PR #1362 added TestExecuteLadderRun_MaxActionsExceeded asserting the old engine contract (Allocate errors when the split exceeds MaxActionsPerRun). This branch changes that contract to deterministic truncation, so it owns the consumer-test update. Renamed to TestExecuteLadderRun_MaxActionsExceeded_TruncatesAndPersists and rewrote it to the new contract: executeLadderRun succeeds, persists a status=planned run whose plan contains exactly one surviving purchase (the larger-gap $70/hr flex allocation) plus a max_actions_per_run hold for the dropped $10/hr base allocation, TotalHourlyCommit reflects only the kept allocation, and tranches exist for it. The fixture ramp moves to a single future step (AfterDays=30) since an AfterDays=0 step becomes a BuyNow action and produces no tranche rows.
This was referenced Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Phase-3 PR-2 of the commitment-laddering automation (tracker #1333, phase-3 umbrella #1336): the
ladder_runscheduled task. This is a PLAN-ONLY milestone. It runs the laddering engine on schedule and persists the plan for audit. It does NOT execute, email, approve, fire reshapes, or create purchase records. Everything stays behind the existing default-offglobal_config.laddering_enabledkill-switch and per-configenabledflag.Contents (T1-T11 of the saved plan)
internal/config):LadderRunDB/LadderTrancheDBtypes mirroring migrations 000080/000081; five methodsSaveLadderRun,GetLadderRun,SaveLadderTranches(batched viaWithTx),LatestLadderRunStartedAt(cadence source),TransitionLadderRunStatus(typed-enum CAS);StoreInterfacewidened + all mocks extended.internal/server):TaskLadderRun = "ladder_run"const +scheduledEventActions+ dispatch tohandleLadderRun(advisory lock automatic via the task lock ID).max(ladder_runs.started_at)per config (daily <20h, weekly <6d20h); per-config error isolation; single-account resolution (Lambda's own account via STS, likehandleRIExchangeReshape); configs on other accounts yield a visibleskipped:multi_account_unsupportedoutcome (fail-loud, not silent).executeLadderRun: read baseline + layer states,Allocate, assemble +ValidateLadderPlan, persist run + audit-onlystatus=scheduledtranches.providers/aws/ladder/factory.go): builds a realAWSLadderfor the resolved account/region, injected via a seam so tests use a fake (never hitting AWS).Design decisions (resolved in the plan)
Single-account (Q1);
Scope.AccountIDfromCloudAccount.ExternalID, the field that holds the 12-digit AWS account number (Q2); direct persistence via the new config methods (Q3); tranches persisted as audit-only, no firing sweep, and verified thatFireScheduledDelayedPurchasessweepspurchase_executionsonly, neverladder_tranches(Q4); purchases + reshapes interleaved in one plan JSON (Q5); cadence gate onmax(started_at)(Q6).Money-path correctness
baseline/target/existing/gap) are*float64nullable and never 0-coerced: a nil-baseline or unavailable-baseline run persistsstatus=plannedwith those columns NULL (Hold-only plan).targetis derived aslow-water * target_coverage/100;gapis the sum of the planned allocation gaps (Σ alloc.GapUSDPerHour, consistent withtotal_hourly_commit), not an ad-hoclow - existing.errored) rather than silently writing a$0row.Verification (all exit 0)
go build ./...,go vet ./...clean.pkg:go test ./ladder/...246 pass.go test ./internal/server/... ./internal/config/...1031 pass.go test -tags=integration ./internal/config/...13 store subtests pass (SaveLadderRun NULL-monetary round-trip,LatestLadderRunStartedAtnil/max,SaveLadderTranchesbatch,TransitionLadderRunStatusCAS win/lose).gocyclo -over 10clean on touched files.Test matrix asserts the plan-only invariant (fake capability
t.Fatals on anyPurchaseLayer/ReshapeBuffer; no email/pending-execution calls), plus flag-off no-op, cadence skip/run, multi-config isolation, nil-baseline/zero-gap Hold, andMaxActionsPerRunfail-loud.The Terraform EventBridge daily tick is intentionally OUT of this PR (moves to PR-5); PR-2 is exercised by a manual
{"action":"ladder_run"}invoke and creates no schedule (stays default-off).Part of #1336. Tracker #1333.
Summary by CodeRabbit
New Features
ladder_runplanning task with global kill-switch, account validation, cadence gating, and per-configuration outcome reporting.Bug Fixes
Tests