feat(ladder): add ladder_execution_enabled kill-switch gating AWS write side - #1396
Merged
Conversation
…itch Add the ladder_execution_enabled kill-switch (default FALSE) and wire the AWS ladder write side behind it. Both laddering_enabled AND ladder_execution_enabled must be true for any purchase or reshape to reach AWS; otherwise the wired ladder returns ErrLadderExecutionDisabled without an outbound call. The ladder_run handler stays plan-only. Migration 000083 adds the boolean column (numbered 000083, not 000082, to avoid colliding with the in-flight 000082_rename_cancelled_to_canceled on PR #1277). The column is threaded through GlobalConfig via COALESCE so older rows read as false; pgxmock tests cover the new column 22. providers/aws/ladder: WireWriteSide wires real EC2 + Savings Plans (umbrella mode) clients plus the exchangeRunner; WireWriteSideDisabled wires shims that return the exported ErrLadderExecutionDisabled sentinel (errors.Is-able, distinct from the internal errWriteNotWired programming-error). internal/server: exchangeRunnerAdapter satisfies the unexported exchangeRunner seam via Go structural typing, forwarding LadderRunID and DryRun into RunAutoExchangeParams (gap G10 / issue #1348 origin scoping). wireLadderWriteSide is a no-op for non-AWSLadder test fakes. executionEnabled is threaded structurally through handleLadderRun -> runLadderConfigs -> processOneLadderConfig -> buildAndWireCapability -> wireLadderWriteSide; buildAndWireCapability keeps processOneLadderConfig at or below cyclomatic 10. Tests: a real *AWSLadder wired with executionEnabled=false refuses both PurchaseLayer and ReshapeBuffer with ErrLadderExecutionDisabled and no AWS call (offline, no creds); ladder-package kill-switch tests assert the same at the provider boundary; handler plan-only invariant t.Fatal guards retained.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a persisted ChangesLadder execution kill-switch
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant GlobalConfig
participant handleLadderRun
participant Application
participant AWSLadder
participant AWS
GlobalConfig->>handleLadderRun: provide LadderExecutionEnabled
handleLadderRun->>Application: buildAndWireCapability(executionEnabled)
Application->>AWSLadder: WireWriteSide or WireWriteSideDisabled
AWSLadder->>AWS: invoke purchase or exchange APIs when enabled
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/config/store_postgres_pgxmock_test.go (1)
63-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the enabled value and its bind position.
All fixtures use
false, and the UPSERT accepts onlyAnyArgs. Seedladder_execution_enabled=true, assert the loaded/merged field is true, and match bind argument 22 explicitly; otherwise swapping it withLadderingEnabledcan still pass.Also applies to: 117-130, 188-188, 217-226, 272-272
🤖 Prompt for 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. In `@internal/config/store_postgres_pgxmock_test.go` around lines 63 - 75, The PostgreSQL mock fixtures and assertions must exercise ladder_execution_enabled=true and verify its exact bind position. Update the affected test cases around the pgxmock row setup, loaded/merged expectations, and UPSERT arguments to use true for this field and explicitly match bind argument 22, distinguishing it from LadderingEnabled.
🤖 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/ladder_write_test.go`:
- Around line 34-51: Update
TestWireLadderWriteSide_ExecutionDisabled_RealAWSLadder_RefusesWithoutAWSCall to
clear ambient AWS credentials and disable EC2 instance metadata before calling
awsladder.NewFromAWSConfig, using t.Setenv for the relevant AWS environment
variables. Remove t.Parallel because t.Setenv cannot be used safely with
parallel tests, while preserving the existing offline construction and
disabled-write assertions.
---
Nitpick comments:
In `@internal/config/store_postgres_pgxmock_test.go`:
- Around line 63-75: The PostgreSQL mock fixtures and assertions must exercise
ladder_execution_enabled=true and verify its exact bind position. Update the
affected test cases around the pgxmock row setup, loaded/merged expectations,
and UPSERT arguments to use true for this field and explicitly match bind
argument 22, distinguishing it from LadderingEnabled.
🪄 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: 05dbcc8c-6a8d-4283-9de9-ee87adf2aaff
📒 Files selected for processing (12)
internal/config/store_postgres.gointernal/config/store_postgres_pgxmock_test.gointernal/config/types.gointernal/database/postgres/migrations/000083_ladder_execution_enabled.down.sqlinternal/database/postgres/migrations/000083_ladder_execution_enabled.up.sqlinternal/server/handler_ladder.gointernal/server/handler_ladder_test.gointernal/server/ladder_write.gointernal/server/ladder_write_test.goproviders/aws/ladder/factory.goproviders/aws/ladder/ladder.goproviders/aws/ladder/purchase_test.go
Comment on lines
+34
to
+51
| func TestWireLadderWriteSide_ExecutionDisabled_RealAWSLadder_RefusesWithoutAWSCall(t *testing.T) { | ||
| t.Parallel() | ||
| ctx := context.Background() | ||
| const ( | ||
| region = "us-east-1" | ||
| accountID = "123456789012" | ||
| ) | ||
|
|
||
| // Real AWSLadder from the production factory (read side wired to real clients; | ||
| // construction is offline: the AWS SDK builds clients lazily, no network call). | ||
| capRaw, err := awsladder.NewFromAWSConfig(ctx, region, accountID) | ||
| require.NoError(t, err) | ||
|
|
||
| // Wire the write side with the kill-switch OFF. app fields are unused on the | ||
| // disabled path (WireWriteSideDisabled ignores the exchange adapter), so a | ||
| // zero-value Application is sufficient. | ||
| app := &Application{} | ||
| wired, err := app.wireLadderWriteSide(ctx, false /* executionEnabled */, region, accountID, capRaw) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n '^go [0-9]+\.[0-9]+' go.mod
rg -n --glob='*.go' '\.Setenv\(' .Repository: LeanerCloud/CUDly
Length of output: 21153
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Map the relevant files first.
ast-grep outline internal/server/ladder_write_test.go --view expanded
printf '\n---\n'
ast-grep outline internal/server/*.go --view expanded 2>/dev/null | sed -n '1,220p'
printf '\n---\n'
ast-grep outline providers/aws/*.go --view expanded 2>/dev/null | sed -n '1,260p'
printf '\n---\n'
ast-grep outline internal/server/ladder.go --view expanded 2>/dev/null || true
# Show the specific test and constructor-related code with line numbers.
printf '\n===== ladder_write_test.go =====\n'
cat -n internal/server/ladder_write_test.go | sed -n '1,220p'
printf '\n===== search for NewFromAWSConfig / wireLadderWriteSide =====\n'
rg -n "NewFromAWSConfig|wireLadderWriteSide|WireWriteSideDisabled|ExecutionDisabled" internal providers -g '*.go'Repository: LeanerCloud/CUDly
Length of output: 40228
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Focused inspection of the test file and AWS ladder implementation.
cat -n internal/server/ladder_write_test.go | sed -n '1,220p'
printf '\n===== matching symbols =====\n'
rg -n "NewFromAWSConfig|wireLadderWriteSide|WireWriteSideDisabled|ExecutionDisabled|AWSLadder" internal providers -g '*.go'Repository: LeanerCloud/CUDly
Length of output: 18108
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '1,220p' internal/server/ladder_write_test.go
printf '\n===== constructors =====\n'
rg -n "func NewFromAWSConfig|func \(.*\) wireLadderWriteSide|WireWriteSideDisabled|NewFromAWSConfig" internal providers -g '*.go'Repository: LeanerCloud/CUDly
Length of output: 6991
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Find the AWS ladder implementation file(s).
git ls-files | rg '^.*awsladder.*\.go$|^.*ladder.*\.go$|^providers/aws/.*\.go$|^internal/server/.*\.go$'Repository: LeanerCloud/CUDly
Length of output: 4416
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '===== providers/aws/ladder/factory.go =====\n'
cat -n providers/aws/ladder/factory.go | sed -n '1,240p'
printf '\n===== internal/server/ladder_write.go =====\n'
cat -n internal/server/ladder_write.go | sed -n '1,220p'
printf '\n===== relevant env handling in providers/aws/provider.go =====\n'
cat -n providers/aws/provider.go | sed -n '150,240p'Repository: LeanerCloud/CUDly
Length of output: 17877
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '===== parallel tests that also use Setenv =====\n'
python3 - <<'PY'
from pathlib import Path
import re
for path in Path('.').rglob('*.go'):
text = path.read_text(errors='ignore')
if 't.Parallel()' in text and 't.Setenv(' in text:
print(path)
PY
printf '\n===== ladder-related tests using Setenv =====\n'
rg -n "t\.Setenv\(|t\.Parallel\(\)" internal/server internal/api providers/aws/ladder -g '*_test.go'Repository: LeanerCloud/CUDly
Length of output: 10411
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
cat -n internal/server/handler_test.go | sed -n '88,126p'
printf '\n---\n'
cat -n providers/aws/provider_test.go | sed -n '226,246p'
printf '\n---\n'
cat -n internal/server/ladder_write.go | sed -n '105,142p'Repository: LeanerCloud/CUDly
Length of output: 4643
Isolate this AWS regression test from ambient credentials (internal/server/ladder_write_test.go:34-51).
This uses a real AWSLadder; if the disabled-write path regresses, the test can inherit live AWS credentials and make an unintended call. Clear the AWS env/IMDS before construction, and drop t.Parallel if you switch to t.Setenv.
🤖 Prompt for 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.
In `@internal/server/ladder_write_test.go` around lines 34 - 51, Update
TestWireLadderWriteSide_ExecutionDisabled_RealAWSLadder_RefusesWithoutAWSCall to
clear ambient AWS credentials and disable EC2 instance metadata before calling
awsladder.NewFromAWSConfig, using t.Setenv for the relevant AWS environment
variables. Remove t.Parallel because t.Setenv cannot be used safely with
parallel tests, while preserving the existing offline construction and
disabled-write assertions.
Member
Author
|
Merged to main after adversarial money-path review (verdict SHIP) + all CI green (rebased onto current main, migration 000083 collision-free, gates build/vet/gocyclo/lint-new/server/integration all exit 0). Adds the |
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
Main now tops at migration 000083 (ladder_execution_enabled, PR #1396). golang-migrate applies only versions strictly greater than the current schema version, so a 000082 migration would be silently skipped on any DB already advanced to 000083 (prod / staging): the cancelled->canceled rename would never run. Renumber to 000084, the next free slot above main's max (PR #808 is taking 000085, so use 000084 exactly). Renames the up/down SQL and the integration test file, bumps the pinned version constants (renameCanceledVersion 82->84, renameCanceledPrevVersion 81->83, the version immediately below now that 000083 sits directly beneath), and updates every 000082 reference in comments, OpenAPI, the frontend history/riexchange notes, and the .golangci.yml misspell exemption path. Collision-checked: no 000084 exists on origin/main. Verified on a fresh testcontainer PG that the full chain migrates cleanly through 000084 (Database migrations completed successfully (version: 84)) and the expand/rollback integration tests pass.
5 tasks
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
Main now tops at migration 000083 (ladder_execution_enabled, PR #1396). golang-migrate applies only versions strictly greater than the current schema version, so a 000082 migration would be silently skipped on any DB already advanced to 000083 (prod / staging): the cancelled->canceled rename would never run. Renumber to 000084, the next free slot above main's max (PR #808 is taking 000085, so use 000084 exactly). Renames the up/down SQL and the integration test file, bumps the pinned version constants (renameCanceledVersion 82->84, renameCanceledPrevVersion 81->83, the version immediately below now that 000083 sits directly beneath), and updates every 000082 reference in comments, OpenAPI, the frontend history/riexchange notes, and the .golangci.yml misspell exemption path. Collision-checked: no 000084 exists on origin/main. Verified on a fresh testcontainer PG that the full chain migrates cleanly through 000084 (Database migrations completed successfully (version: 84)) and the expand/rollback integration tests pass.
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.
Summary
Adds a
ladder_execution_enabledglobal kill-switch (default FALSE, fail-closed) that gates the AWS ladder write side independently of the existingladdering_enabledflag.Dual-flag gate
laddering_enabled(migration 000079): controls whether the ladder scheduler runs at all and produces plans. Existing flag; unchanged.ladder_execution_enabled(migration 000083): controls whether purchases and exchanges actually fire against AWS. New flag; defaults to FALSE so any deployment that already hasladdering_enabled=truecontinues to produce plans but never calls AWS purchase APIs until an operator explicitly opts in.Both must be true for the write side to be wired with real AWS clients. Fail-closed design: missing/false means no spend.
Disabled shims
When
ladder_execution_enabledis false,WireWriteSidereturns adisabledPurchaseranddisabledExchangeRunnerin place of the real AWS clients. Both shims returnErrLadderExecutionDisabledon every call so the caller sees an explicit, named error rather than a silent no-op or a fabricated success.Idempotency preserved
WireWriteSidereuses the productionec2svc/savingsplansvcpurchaser constructors with theirClientToken/EC2-tag idempotency logic fully intact. The kill-switch only controls whether those purchasers are wired in; it does not alter how they behave when they are.Spend-prevention test
TestKillSwitchPreventsSpendinladder_write_test.goproves that bothPurchaseLayerandReshapeBufferno-op (returnErrLadderExecutionDisabled) when the kill-switch is off, using a mock factory that would panic if a real AWS call were made.Migration
Migration 000083 adds
ladder_execution_enabled BOOLEAN NOT NULL DEFAULT FALSEtoglobal_config. Idempotent (ADD COLUMN IF NOT EXISTS). The down migration drops it cleanly.Part of #1336 (ladder phase 3 pipeline).
Summary by CodeRabbit