Skip to content

fix(server): report the ladder plan's net existing commitment - #1461

Merged
IAmJSD merged 5 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-243-rest
Oct 28, 2026
Merged

IAmJSD merged 5 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-243-rest

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Why

#243's last open item (finding 8, cross-repo): assembleLadderPlan added up each layer's gross ExistingUSDPerHour and wrote that total into the ladder plan, ladder_runs.existing_usd_hr and the approval email's Explain() output. Allocate, though, sizes the gap against net existing commitment (existing minus expiring). Take a layer with $8/hr OD existing, of which $4/hr expires, and a $10/hr target. The plan showed Existing $8/hr next to a $6/hr gap, so an approver could not get from target minus existing to the gap.

The go side landed in LeanerCloud/cloud-commitments-go#614: AllocateResult.NetExistingUSDPerHour. main already pins cloud-commitments-go at 88b95b6 (#1490), which contains go#614, so this PR no longer changes the pin. It is now only the handler change and its test.

The other #243 findings were fixed earlier: #1265 has the per-finding table and lists finding 8 as the only deferred item. I re-checked them on current main. So this PR closes the issue.

Closes #243

What

fix(server): assembleLadderPlan now reports allocResult.NetExistingUSDPerHour (copied, not aliased) and no longer takes the layerStates parameter. Allocate sets this figure on every successful result, hold-only exits included. It covers only the configured layers, but Allocate already rejects a layer state outside them, so the scope is the same as before.

Files: internal/server/handler_ladder.go and the new test internal/server/ladder_existing_units_test.go.

Refresh note. The branch originally carried a chore(deps) commit bumping the pin to go 80e9b846 plus the follow-ups that bump needed (provider.ProviderConfig to provider.Config rename, the computeengine.NewClient signature change, the bypass-guard update). #1490 has since moved main to 88b95b6, which contains 80e9b846 (checked with git merge-base --is-ancestor), and brought the same follow-ups. origin/main was merged into this branch with go.mod, go.sum, internal/providerfactory/bypass_guard_test.go and the leftover comment in internal/purchase/execution.go all taken from main, so git diff origin/main...HEAD now shows only the two files above. The go behaviour changes that came with the bump reached main through #1490 and are not part of this PR.

Verification

Fail before, pass after. The new test is TestExecuteLadderRun_ReportsNetExistingCoverage in internal/server/ladder_existing_units_test.go. It runs the real executeLadderRun → Allocate → plan pipeline with existing 4 and expiring 2 commitment at rate 0.5 (8 OD gross, 4 OD net). There are two cases: an allocating run and a baseline-unavailable hold.

  • With origin/main's handler_ladder.go, both cases FAIL: Max difference between 4 and 8 on existing_usd_hr and on the plan JSON, and gap reconciles with target - existing fails with 2 vs 6.
  • With this PR, both cases PASS.

Checks after the refresh (run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true):

  • go build ./..., go vet ./... and go vet -tags integration ./...: clean.
  • go test ./internal/server/ ./internal/providerfactory/: pass.
  • golangci-lint run ./internal/server/...: 0 issues.

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 400d256b = origin/main 7b86e386 plus these heads, merged in this order:

Branch Tested head
fix/issue-243-rest 007c163
fix/issue-1206 312072f
fix/issue-111-email 6d422e2
fix/issue-111-fail-closed 3817e29
fix/issue-712-age-cap a91120b
fix/issue-712-fargate aeaf0d4
fix/issue-613-legacy-ids 350e837
fix/issue-120-guards 6f38ebc
fix/issue-120-shared-predicate cb25735
fix/issue-151-account-update-keep d20cf01
fix/issue-713-upcoming-filter e73fdfb
fix/issue-263-bastion-opts 7861f32
fix/issue-43-fixture-ids 8cee203
fix/issue-177-makefile-walk 365a9d6
fix/issue-27-seller-not-registered d845f66
fix/issue-313-user-columns 6d759dd
fix/issue-353-mock-panics aac2edf
fix/issue-8-config-stamps b548ae5
fix/issue-30-monthly-totals f637921

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran on f7c48020, which differs from this tree only by test-only commits on fix/issue-30, fix/issue-713 and fix/issue-263 that replace placeholder account-number literals with named constants:

  • Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
  • Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
  • Integration tests, every changed package that has them, one package at a time (-tags integration -count=1): internal/analytics, internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase, internal/scheduler and internal/server all pass. Migrations 000149, 000150 and 000151 apply in order on this tree.
  • Frontend: the changed jest files (approval-details, settings-accounts, api) pass (221 tests).
  • Secrets: the first run flagged 9 new fixture lines (placeholder account numbers next to an account key); fixed with named constants. Re-run on this tree: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.
  • Terraform .tftest.hcl files were not run locally (CI runs them). Nothing was applied to any cloud.

🤖 Generated with Claude Code

Amendment ceiling

New ladder runs also use the net existing commitment when enforcing tranche-amendment raises. The ceiling therefore increases by the expiring amount relative to the former gross snapshot, matching the planner allocation. Older stored runs keep their gross, stricter ceiling until a new run replaces the snapshot. The implementation comment now describes both cases.

IAmJSD and others added 2 commits October 26, 2026 08:57
The platform pinned pkg to a commit from before go#614, so Allocate's
NetExistingUSDPerHour was not available to the ladder handler (LeanerCloud#243).
Move pkg and providers/{aws,azure,gcp} to go main, which also brings in
go#441, LeanerCloud#471, LeanerCloud#491, LeanerCloud#439, LeanerCloud#627, LeanerCloud#628 and LeanerCloud#629.

The bump needs three mechanical follow-ups:
- go#575 dropped the ctx argument from computeengine.NewClient; the GCP
  history-cost test is updated to the new signature.
- go#603/LeanerCloud#609 renamed provider.ProviderConfig to provider.Config and left
  the old name as a deprecated alias. staticcheck SA1019 flags every use,
  so all references move to provider.Config. mock.AnythingOfType reports
  the real type name, so the zonal-recs mock now matches
  "*provider.Config".
- The providerfactory bypass guard matched only the ProviderConfig name;
  it now recognises both names so a Profile literal is still caught.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
assembleLadderPlan summed each layer's gross ExistingUSDPerHour into the
plan, ladder_runs.existing_usd_hr and the approval email, while Allocate
sized the gap against existing minus expiring. A layer with $8/hr OD of
existing of which $4/hr expires showed Existing $8/hr next to a gap of
$6/hr against a $10/hr target, which does not reconcile.

Report Allocate's NetExistingUSDPerHour instead (go#614), which is set
on every result including the hold-only exits, so the plan shows the
figure the engine used rather than a recomputed one.

Closes LeanerCloud#243

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added effort/l Weeks type/chore Maintenance / non-user-visible priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/few Limited audience triaged Item has been triaged labels Oct 26, 2026
@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 8b64a354ce1c8a6fb82bae44c93b55cc86049a2c

Blocking:

  • none

Non-blocking:

Attacked:

  • Ladder change: read Allocate at the new pin. NetExistingUSDPerHour is set on every successful return, hold-only exits included, and is a copy. The plan copies it again, so there is no aliasing. A nil existing is unreachable on success. Other readers of existing_usd_hr: the timeline (display only) and the amend ceiling (above).
  • Fail-before: put main's internal/server/handler_ladder.go back in the scratch tree, at the new pin. TestExecuteLadderRun_ReportsNetExistingCoverage fails in both subtests (Max difference between 4 and 8 on the run and on the plan JSON; gap 2 vs 6). Restored, it passes.
  • Bump hygiene: go mod tidy -diff is clean, go mod verify passes, and go.sum changes only the 8 cloud-commitments-go lines (no transitive bumps). The provider.ProviderConfig -> provider.Config rename is a type alias. The bypass guard now matches both names.
  • Ran: go build ./...; go vet ./..., with and without -tags integration, plus e2e; go test -race -short -count=1 ./... (every package ok); go test -race -tags integration -count=1 on server, scheduler, providerfactory, purchase, api and config (all ok); golangci-lint v2.10.1 on the changed packages, with and without the integration tag (0 issues); gocyclo -over 10 on the changed non-test files (clean). For Closes chore(purchase/ladder/exchange): medium/low findings from the 2026-07-28 full review [platform part] #243, I spot-checked the other items on main (ctx-aware finalize sleep and its test, the PlanID == "" short-circuit, the findAWSAccount Enabled filter, 503 on a validation error, positive confirmation in the Azure probe). All are present, and fix(purchase,commitmentopts): close out the open #243 review findings #1265 is merged.

Body-stale:

  • none

Reviewer: rE2. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 8b64a354ce1c8a6fb82bae44c93b55cc86049a2c

Blocking:

  • none

Non-blocking:

Attacked:

  • Code: assembleLadderPlan now reads AllocateResult.NetExistingUSDPerHour. In go pkg/ladder/allocate.go:175 that is set on every successful result, so it's never nil after a nil error, and it's copied rather than aliased. Dropping the layerStates parameter loses nothing, because Allocate already rejects out-of-scope layer states.
  • Fail-before: with main's handler_ladder.go and the new pin, both subtests of TestExecuteLadderRun_ReportsNetExistingCoverage FAIL (4 vs 8 on existing_usd_hr and the plan JSON, 2 vs 6 on gap reconciliation). With the PR they PASS. Restored afterwards.
  • Bump fallout I checked against the platform's callers:
  • Ran: go build ./..., go vet ./..., go vet -tags integration ./..., tests/e2e vet with -tags e2e, go mod verify (all verified), go mod tidy (no diff). golangci-lint v2.10.1 on ./... and with --build-tags integration on the changed packages: 0 issues. Full go test -race -short -count=1 ./...: all ok. go test -race -tags integration -count=1 on internal/server, purchase, scheduler and providerfactory, plus -tags integration ./internal/api/: all ok.

Body-stale:

Reviewer: rE6. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 8b64a354ce1c8a6fb82bae44c93b55cc86049a2c

Addendum to my review above. After posting, I skimmed the earlier reviews and found one point I had missed. It comes from an earlier review, and I confirmed it in the code:

Blocking:

  • none

Non-blocking:

  • (from an earlier review) internal/config/store_postgres_ladder_amend_ceiling.go:30-35 and :116: the tranche-amend raise ceiling is LEAST(target, baseline*coverage/100) - existing_usd_hr, taken from the latest run. This PR changes that column from gross to net of expiring, so on new runs the ceiling is looser by the expiring amount, and the comment "existing is gross, so C errs low" is now wrong. I agree the net value is the consistent one, because it's the room Allocate sized the gap against. Still, it's a cap change on a money path. The comment should be updated, and the PR body should say the amend ceiling follows the column. Older ladder_runs rows keep the gross value, which is stricter, until the next run.

Attacked: read checkLadderConfigCeiling / loadLadderCeiling at this head to confirm they consume existing_usd_hr.

Body-stale: as in my review above. Also, the body doesn't mention the amend-ceiling change.

Reviewer: rE6. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.

# Conflicts:
#	go.mod
#	go.sum
#	internal/providerfactory/bypass_guard_test.go
@IAmJSD IAmJSD changed the title fix(server): report the ladder plan's net existing commitment (bump cloud-commitments-go) fix(server): report the ladder plan's net existing commitment Oct 28, 2026
IAmJSD and others added 2 commits October 28, 2026 00:48
govulncheck in the Docker image build now fails on these two advisories,
both fixed in x/net v0.62.0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 3a1a0757b32378a0daf9ee45665ea3980405aa68

Blocking:

  • none

Non-blocking:

  • Money-path reach. ladder_runs.existing_usd_hr isn't display-only. loadLadderCeiling (internal/config/store_postgres_ladder_amend_ceiling.go:113-117) computes the tranche-amend raise ceiling from it, so this PR raises that ceiling by the expiring amount. I agree the result is right: it now equals the planner's own bound (min(target, lowWater) - E), and the head's docs(ladder) commit plus the "Amendment ceiling" body section disclose it. Still, the PR now changes a money-path cap, so I'd hold it to the two-review money bar rather than the one-review bar. (Earlier reviews on 8b64a354 raised the same point, and I found it independently. Their note about the stale comment is now resolved by 3a1a0757.)
  • No end-to-end test of the ceiling's new input. No test pins that a run's net figure is what the amend ceiling consumes. TestLadderAmendCeiling* seeds existing_usd_hr directly, and its subtest "gross existing lowers the ceiling" (store_postgres_ladder_amend_ceiling_integration_test.go:143) now describes a state that only pre-upgrade runs produce. Renaming it, or adding a case "net existing from a run with expiring commitment allows raising up to target - net", would document the intent.
  • "Reconciles" holds only in the simple case. target - existing = gap holds for a total-basis baseline with no in-flight tranches. That's the case the test covers. In general, Allocate sizes gap = min(target-E, lowWater-E) - inFlight, with E = 0 for a residual baseline. So the plan can still show a gap that doesn't equal target - existing whenever scheduled tranches are in flight or the baseline is residual. The assembleLadderPlan comment already says gap is NOT target - existing, which is correct. The PR body's framing ("an approver could not get from target minus existing to the gap") is just slightly broader than the fix.

Attacked:

  • Read Allocate/computeNetExisting at the pinned go 88b95b6. NetExistingUSDPerHour is set on every successful result, including the hold-only exits, so the nil branch in assembleLadderPlan is unreachable in practice and safe if reached. The value is copied, not aliased.
  • Grepped every consumer of ExistingUSDPerHour / existing_usd_hr: the plan DTO, ladder_runs, the timeline read and the amend ceiling. This is how I found the ceiling dependency above.
  • Checked the Closes #243 claim. fix(purchase,commitmentopts): close out the open #243 review findings #1265 (merged) carries the per-finding table, and I spot-checked several of those fixes on main: findAWSAccount sets Enabled, plus autoPurchasePlan, markRevokedWithRetry, walkPaginatedCapped, the 503 in checkCommitmentOptionCombo and finalize_revocations_test.go. Finding 8 is the last item, so Closes is justified.
  • Fail-before: with origin/main's handler_ladder.go, TestExecuteLadderRun_ReportsNetExistingCoverage FAILS in both subtests (4 vs 8 on existing_usd_hr and in the plan JSON; gap reconcile 2 vs 6). I restored the file afterwards.
  • Ran on 007c163e: go build ./...; go vet with and without -tags integration on internal/server; golangci-lint v2.10.1 with and without --build-tags integration (0 issues); gocyclo -over 10 on handler_ladder.go (silent); go test -race -short -count=1 ./internal/server/ (ok); go test -race -tags integration -count=1 ./internal/server/. The first run, at load around 19-30, FAILED after 427s. My captured tail didn't include the failing test, only scheduled-task lock noise, so I can't name it. A serial rerun once the box was idle, on head 3a1a0757, passed in 30s. I read the first failure as load-induced, per the shared-VM rule, but I haven't proved it.
  • The head moved to 3a1a0757 while I was reviewing. It adds the x/net bump (identical to fix(deps): bump golang.org/x/net to v0.62.0 (GO-2026-6860, GO-2026-6866) #1564) and the comment-only docs(ladder) commit. On 3a1a0757 I re-ran go build ./..., go vet ./..., go vet -tags integration ./internal/server/ ./internal/config/ and go test -race -short -count=1 ./internal/server/ ./internal/config/, and all pass.

Body-stale:

  • none

Reviewer: rG1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.

@IAmJSD

IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 3a1a0757b32378a0daf9ee45665ea3980405aa68

Blocking:

  • none

Non-blocking:

  • Money cap, for the owner to see. The amendment ceiling C = min(target, baseline*cov%) - existing_usd_hr (internal/config/store_postgres_ladder_amend_ceiling.go:118) gets looser by the expiring amount on every new run. I checked that this is safe, not just a change. Allocate sizes gap = target - netE - inFlight, so after a run the config's in-flight total is about target - netE.
    • With gross existing, C = target - grossE = (planner gap) - expiring. The planner's own tranches already exceeded that ceiling, so any raise was refused whenever something was expiring.
    • With net existing, C equals the planner's own sizing. An amendment can raise only up to what the approved plan already allotted, never past it.
    • Older runs keep the gross, stricter figure, as the updated comment says.
    • The ceiling SQL is unchanged.
    • Suggestion: add an integration case to store_postgres_ladder_amend_ceiling_integration_test.go that pins "a raise up to target - net existing passes, one past it is refused". The new behaviour would then be locked down at the ceiling and not only at the snapshot.
  • Display: frontend/src/ladder-timeline-chart.ts labels the series built from run.existing_usd_hr "Existing commitment", and ladder-timeline.ts prints "existing $X" in the run snapshot. Both now show the net figure. "Existing commitment (net of expiring)" would save an operator from comparing it against the provider console's gross number. The approval email's Explain() text has the same issue.
  • The branch still has its own copy of the x/net v0.62.0 bump (e3e95bf). Main already has it via fix(deps): bump golang.org/x/net to v0.62.0 (GO-2026-6860, GO-2026-6866) #1564, so it will drop out on the next main merge.

Attacked:

  • assembleLadderPlan now copies allocResult.NetExistingUSDPerHour (new big.Rat, no aliasing) and no longer takes layerStates. In the pinned go module (88b95b6), Allocate sets the field on every successful result, including the hold-only exits. computeNetExisting errors on a negative per-layer net (expiring > existing), so the ceiling can't be inflated by a bad snapshot. validateNoUnknownStateKeys rejects layer states outside the configured layers, so the scope matches the old gross sum. I grepped every consumer of existing_usd_hr: the ceiling, the timeline and the cancel/list queries. Only the ceiling uses it for money, as covered above.
  • Fail-before: with internal/server/handler_ladder.go put back to main's version, TestExecuteLadderRun_ReportsNetExistingCoverage fails in both subtests (Max difference between 4 and 8, and gap 2 vs 6 on reconcile). Restored afterwards.
  • Ran go build ./..., go vet ./... and go vet -tags integration ./...: clean. Ran go test -race -short -count=1 ./internal/server/ ./internal/config/: ok. Ran go test -race -tags integration -count=1 on ./internal/config/ (includes the amend-ceiling integration tests) and ./internal/server/: both ok. CI on this head: 39 pass, 2 skipping.
  • "Closes chore(purchase/ladder/exchange): medium/low findings from the 2026-07-28 full review [platform part] #243": fix(purchase,commitmentopts): close out the open #243 review findings #1265 lists finding 8 as the only deferred item, and this PR is that item.

Body-stale:

  • none

Reviewer: rH3. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.

@IAmJSD

IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at 3a1a0757b32378a0daf9ee45665ea3980405aa68 merged onto main (b85e42a0), together with #1461, #1557, #1563 in that order (combined tree 4d91ad42, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 28, 2026 03:23
@IAmJSD
IAmJSD requested a review from cristim as a code owner October 28, 2026 03:23
@IAmJSD
IAmJSD merged commit 1d1bece into LeanerCloud:main Oct 28, 2026
41 checks passed
@IAmJSD

IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on 3a1a0757, green CI, and the combined-tree verification above. Squash-merged at exactly 3a1a0757b32378a0daf9ee45665ea3980405aa68.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/l Weeks impact/few Limited audience priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore(purchase/ladder/exchange): medium/low findings from the 2026-07-28 full review [platform part]

1 participant