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

Split from LeanerCloud/cloud-commitments-cli#1583 (cross-component issue, monorepo split)

This is the internal/-scoped half of the original tracking issue. The pkg/ladder, pkg/exchange, and providers/aws/ladder findings from the same review are split into LeanerCloud/cloud-commitments-go (see the "Cross-repo" bullet below for the one item that spans both repos).

Reviewed commit: be11bdcb5. Note: origin/main moved during the review; re-verify current line numbers before changing code, since a finding may have been fixed or moved since.

Tracking issue for the MEDIUM and LOW findings from the 2026-07-28 money-core review. The CRITICAL and HIGH findings from the same pass are filed as individual issues.

Each checklist item is independently actionable. Please keep the file:line, failure scenario and fix direction with the item when splitting any of these out.

• The reaper (10m) always preempts the idempotent re-drive sweep (15m), so RecoverStrandedApprovals' safe auto-recovery is dead code in production (MEDIUM)

  • Where: internal/purchase/reaper.go:25 (stuckStatuses = {"approved","running"}) and :39 (DefaultReapAfter = 10m) vs internal/purchase/manager.go:94 (staleApprovedThreshold = 15m) and :453-491; both wired as scheduled tasks at internal/server/handler.go:186 and :316
  • What: the reaper CASes approved -> failed at 10 minutes. RecoverStrandedApprovals only selects rows still in approved and only at 15 minutes, so by the time it looks, the reaper has already moved every candidate to failed. The manager.go:86-93 comment asserts the gap is intentional, but does not state the consequence: the whole allRecsSafeToRedrive / claimAndRedrive in-place re-drive path (issue fix(docker): report migrate failures from the entrypoint #639) never fires.
  • Failure scenario: a multi-account purchase strands in approved (Lambda killed mid-run). At T+10m the reaper flips it to failed with "; safe to retry" appended (reaper.go:224-228). The in-place re-drive that would have reused the identical lineage key never runs; the operator instead uses Retry, which mints a new row, and when the stranded row is a per-account fan-out row that is exactly the CRITICAL double-buy filed separately from this review (fix(plans): disable the hidden custom-ramp inputs so a preset ramp can be saved #1537). The safe path is unreachable and the unsafe path is the one the reaper's own message recommends.
  • Fix: either exclude rows younger than staleApprovedThreshold from the reaper's approved arm, or move the re-drive into reapOne so recovery happens on the row that is already claimed.

• A ladder run has no claim/CAS: overlapping ticks both pass the cadence gate and both append tranches (MEDIUM)

  • Where: internal/server/handler_ladder.go:239-249 (cadence gate reads LatestLadderRunStartedAt), then :285-368 (plan), then :437-465 (SaveLadderRunWithTranches insert); internal/config/store_postgres_ladder.go:341-360 is an unconditional insert
  • What: ladderWithinCadenceWindow is a plain read of MAX(started_at); the new run row is only inserted at the very end of persistLadderRun, after the Cost Explorer / CloudWatch / layer-state fetches and Allocate. There is no TransitionLadderRunStatus-style claim and no unique constraint keyed to the cadence window. This is the exact shape that issue fix(plans): guard delete, reschedule and resume of managed plans #1013 fixed for purchases with claimAndExecute.
  • Failure scenario: the ladder_run scheduled task overruns its cadence (the multi-region read side takes minutes) and EventBridge delivers a second invocation, or two Lambdas process the same tick. Both read latest = yesterday, both pass the daily gate, both call GetInFlightLadderCommitUSDHr (which still returns the pre-run total), both compute the same gap G, and both append a full ramp of tranches for G. ladder_tranches now carries 2xG of scheduled commitment for one config, and the next run's netting treats all of it as legitimately in flight, so the over-commitment is never reconciled: it just fires as double the intended purchase once the tranche executor lands.
  • Fix: insert the run row (or take a per-config advisory lock / CAS on a running marker) before the read side, and let the cadence gate CAS against it.

• recIsSafeToRedrive's Azure Savings-Plans exclusion matches two literals out of a ten-spelling slug space, and its stated justification is stale (MEDIUM)

  • Where: internal/purchase/manager.go:290-307 (return rec.Service != "savingsplans" && rec.Service != "savings-plans") vs internal/purchase/execution.go:1102-1117 (mapSavingsPlansSlug, which recognises ten SP slugs: savings-plans, savingsplans, and {savings-plans,savingsplans}-{compute,ec2instance,sagemaker,database}); the same predicate also drives the reaper's "; safe to retry" annotation at internal/purchase/reaper.go:224
  • What: the gate that decides whether a stranded execution may be re-driven is a two-literal string comparison, while the purchase dispatcher in the same package accepts ten spellings for the same concept. Any Azure rec whose Service carries a per-product SP slug falls through to return true and is classified safe to re-drive. Two separate problems compound here:
    • The slug space mismatch (the defect): the exclusion is expressed in raw literals rather than the normalised service type the dispatcher itself uses, so it applies to two of ten spellings arbitrarily.
    • The justification is stale: the block comment at manager.go:266-269 reads "Azure savings-plans: the OrderAlias API uses time.Now().UnixNano() as the alias name; there is no server-side idempotency key and no tag-based lookup implemented yet. Re-driving would create a duplicate savings plan." That is no longer accurate. At be11bdcb5 the alias name is common.ReservationOrderID(opts.IdempotencyToken, fmt.Sprintf("cudly-%d", time.Now().UnixNano())) (in the Azure Savings Plans client in the cloud-commitments-go repo), the deterministic derivation landed for fix(api): guard legacy plan updates with a version token #1079 in PR fix(iac): require a UUID or 32-hex external ID in the CFN cross-account templates #1045. The timestamp is now only the empty-token CLI fallback.
  • Failure scenario: an Azure Savings-Plan rec is persisted with Service = "savingsplans-compute" (a spelling mapSavingsPlansSlug explicitly supports). The execution strands in approved. recIsSafeToRedrive sees provider "azure" and a service that is neither excluded literal, returns true, and RecoverStrandedApprovals calls claimAndRedrive. If the re-drive carries a non-empty IdempotencyToken the alias name is now deterministic and the PUT reuses the existing order alias, so no duplicate is created. If the token is empty, the fallback mints a fresh timestamp alias and a duplicate Azure savings plan is created and charged. Either way the gate is deciding on a rule it does not actually express, and the outcome depends on a field the gate never inspects.
  • Latent but armed, do not close as unreachable: no such rec can be produced today because the Azure SP recommendation client is a stub that returns nothing (in providers/azure/recommendations.go in the go repo, tracked by fix(api): match history rows to scoped accounts by provider and external id #593). The moment that stub is implemented the gate goes live in whatever state it is left in, with no separate signal that it needs revisiting.
  • Fix: test the normalised service type (route through mapSavingsPlansSlug or a typed ServiceType) rather than two raw literals. Then decide the Azure Savings Plans arm deliberately given PR fix(iac): require a UUID or 32-hex external ID in the CFN cross-account templates #1045: either mark it safe conditional on a non-empty idempotency token, or keep it unsafe for everything that normalises to a Savings Plans type. Update the stale comment either way. Related: Do not add the CodeRabbit status to required checks: it reports success while throttled #85 (normalising the savings-plans / savingsplans identifier repo-wide) and fix(api): guard legacy plan updates with a version token #1079. All files touched by the fix (internal/purchase/manager.go, internal/purchase/execution.go, internal/purchase/reaper.go) are in this repo; the Azure client/recommendations files referenced above are context only, in cloud-commitments-go.

• Canonical-slug EC2 recs never get offering_class stamped at all (MEDIUM)

  • Where: internal/purchase/execution.go:838-840 (if rec.Provider == string(common.ProviderAWS) && rec.Service == string(common.ServiceEC2)), with common.ServiceEC2 = "ec2" and common.ServiceCompute = "compute" defined in the shared pkg/common types (cloud-commitments-go repo); slug mapping at execution.go:1060-1078 (mapServiceSlug)
  • What: the guard matches only the legacy AWS-only slug "ec2". mapServiceSlug documents "compute" as the canonical slug for the same service and routes both to a working purchase, so a canonically-slugged AWS EC2 rec purchases fine but skips the stamp entirely, leaving offering_class empty on the history row.
  • Failure scenario: a rec collected with the canonical Service = "compute" buys an EC2 RI. savePurchaseHistory evaluates "compute" == "ec2", gets false, and writes no OfferingClass. The marketplace-sell guard (issue sec(iac/aws): two Fargate EventBridge roles have unconditioned RunTask and PassRole #292) that reads this column specifically to avoid an extra AWS round-trip now sees an empty class for a CUDly-purchased RI and cannot classify it, so the column's stated purpose is defeated for every rec using the slug the codebase considers canonical.
  • Fix: match on the normalised service type (m.mapServiceType(rec.Service) against common.ServiceCompute or common.ServiceEC2) rather than one raw legacy literal. Combine with the separately filed HIGH on the same three lines (the stamped value is a hardcoded "convertible" even when a Standard RI was purchased) so both are fixed in one change.

• commitmentopts.Validate can hard-reject a legitimate (term, payment) combo for OpenSearch and Redshift (MEDIUM)

  • Where: internal/commitmentopts/probe.go:23 (maxPages = 5), :271-297 (OpenSearch fetches unfiltered then filters client-side for t3.small.search), :326-352 (Redshift, same shape for dc2.large), internal/commitmentopts/service.go:149-184 (Validate returns (false, nil) on a miss), cache scope at service.go:76-124
  • What: Validate is permissive when a service key is absent from the probe data, but strict when the key is present with a partial combo list. For OpenSearch and Redshift the AWS API offers no instance-type filter, so the probe pulls at most 5x100 offerings and filters afterwards. A partial matrix is a normal outcome, not an error, and the code cannot distinguish "AWS does not sell this" from "the page cap cut it off".
  • Failure scenario: in a region where DescribeReservedInstanceOfferings returns 1-year no-upfront t3.small.search offerings on page 2 and the 3-year all-upfront ones on page 7, the probe persists only {1yr, no-upfront} for opensearch. A user configuring a legitimate 3-year all-upfront OpenSearch plan hits Validate -> (false, nil) and the save is rejected with no way to tell it apart from a genuinely unsold combination. The cached snapshot is also taken from one account in one region and applied to every region.
  • Fix: record whether the probe terminated because pagination was exhausted or because the page cap fired, and fall back to permissive (with a warning) on a capped or partial result, matching the treatment already given to a fully-absent service key.

• A pending direct-execute row (PlanID == "") makes every scheduler tick record a permanent failure (LOW)

  • Where: internal/purchase/manager.go:507-520 (executableByScheduler calls GetPurchasePlan(ctx, exec.PlanID) unconditionally), :533-538 (a gate error is counted Failed); direct-execute PlanID == "" documented at internal/purchase/execution.go:38-47
  • What: GetPurchasePlan("") hits a WHERE id = $1 against a UUID column and returns a SQLSTATE 22P02 error, which executableByScheduler wraps and processOneExecution counts as a failure, forever, on every tick, because nothing transitions the row.
  • Failure scenario: a direct-execute purchase whose synchronous run aborted before the status flip is left pending with PlanID="" and a due ScheduledDate. Every process_scheduled_purchases tick logs "AutoPurchase gate check failed" and increments ProcessResult.Failed, poisoning the task's error signal so a genuine failure is indistinguishable from the stuck row.
  • Fix: short-circuit executableByScheduler to (false, nil) when exec.PlanID == "" (a plan-less row can never be auto-purchased by the plan gate anyway), matching the guard updatePlanProgress already has at execution.go:1126-1131.

• FinalizeInFlightRevocations sleeps without honoring context cancellation (LOW / re-scored HIGH by the 2026-09-02 audit as A16-005, see below)

  • Where: internal/purchase/finalize_revocations.go:56-64 (time.Sleep(backoff) inside the retry loop)
  • What: the retry loop sleeps up to 8s per row with a bare time.Sleep, ignoring ctx. With N stuck rows the sweep can burn 8N seconds past a cancelled context.
  • Failure scenario: 30 rows are stuck because the DB is down; the Lambda's remaining budget is 60s. The sweep sleeps roughly 240s of wall clock past the deadline instead of returning promptly, so the invocation is killed mid-loop and the FinalizeResult counts are never logged: the operator loses the signal that revocation finalization is failing at all.
  • Fix: replace with select { case <-ctx.Done(): return ...; case <-time.After(backoff): } and abort the sweep on cancellation. Matches feedback_go_ctx_aware_patterns.

Cross-repo item (also filed in cloud-commitments-go)

• Ladder plan reports Existing gross of expiring commitment, inconsistent with the E used to size the gap (LOW)
  • Where (this repo): internal/server/handler_ladder.go:575-582 (existingTotal += *ls.ExistingUSDPerHour) is the display bug; consumes computeNetExisting in pkg/ladder/allocate.go (cloud-commitments-go repo).
  • What: Allocate (go repo) sizes the gap against net existing commitment; assembleLadderPlan (this repo) reports the gross figure into plan.ExistingUSDPerHour, ladder_runs.existing_usd_hr, and the approval-email Explain() output.
  • Failure scenario: a layer with $10/hr existing of which $4/hr expires next month shows Existing: $10.00/hr in the plan while the engine used $6/hr. The approver reading the email sees a gap that does not reconcile with the reported target minus existing, and cannot tell whether the engine double-counted expiring commitment.
  • Fix: this repo's assembleLadderPlan needs to consume the same net figure computeNetExisting produced, once the go repo threads it out of Allocate's return value instead of it being recomputed here from layerStates. Coordinate with the sibling issue in cloud-commitments-go since the fix requires a signature change on that side first.

Note on #1398 (buy-now ramp steps escape netting, related to the item above)

That finding (about internal/server/handler_ladder.go reporting the full allocation gap into the approval plan while persistLadderRun only persists delayed tranches) was re-scored HIGH on the same 2026-07-28 pass and tracked on LeanerCloud/cloud-commitments-cli#1398 directly, not repeated here. It touches the same handler_ladder.go area as the two bullets above, so fix them together if convenient.

Findings from the 2026-09-02 codebase audit

Added by an automated audit of the codebase state at the time. Each item below was reported by one reviewer and independently confirmed by a second that did not write it.

A16-005 (high)

The ctx bullet above (FinalizeInFlightRevocations) has a companion worth landing with it: finalize_revocations.go has no test at all. There is no finalize_revocations_test.go, nothing in internal/purchase references the method, and the only exercise of the feature is internal/server/handler_test.go:142/152, which installs a fake returning FinalizeResult{Found: 1, Finalized: 1} and then asserts only that HandleScheduledTask returned no error, an assertion on the fake's own return value that holds whatever the real sweep does. Every statement of lines 46-75 is 0 in a merged cross-package profile. So the ctx-blind time.Sleep(backoff) at :62 (two backoffs, up to 8s per stuck row) can be fixed with nothing to prove it, and the Found/Finalized/Errored accounting can be wrong in any direction while the suite stays green, on the rows that are the audit record for Azure reservations already returned to the provider. A table test over MockConfigStore (one row succeeding first try, one on retry, one never) plus a cancelled-context case once the sleep is ctx-aware would cover both halves. (audit findings A16-005 and A05-007)

A05-010 (medium)

There is an earlier failure on this path that the probe-cap analysis (the commitmentopts.Validate bullet above) does not reach. findAWSAccount (internal/commitmentopts/service.go:127-136) documents itself as returning "the first enabled AWS account" but builds config.CloudAccountFilter{Provider: &provider} and leaves the Enabled *bool field nil, so ListCloudAccounts returns disabled rows too and the loop takes the first by index. An operator who disables a decommissioned AWS account but leaves the row in place has its revoked credentials used for the probe; buildConfig or the probe call then fails, probeAndPersist returns ErrNoData (service.go:86), and Validate falls permissive-true forever even though a healthy enabled account sits later in the list. The sibling Scheduler.enabledAccounts (internal/scheduler/scheduler.go:916-920) sets both filter fields; copying that here also makes the in-loop Provider == "aws" re-check redundant. Audit finding A05-010.

A02-022 (low)

The commitmentopts.Validate bullet has a counterpart failing in the other direction. checkCommitmentOptionCombo (internal/api/handler_config.go:177-181) logs and returns nil when commitmentOpts.Validate errors (DB blip, timeout), so updateServiceConfig proceeds to SaveServiceConfig (:356-362) and a PUT /api/config/service/aws/rds with an unsupported (term, payment) pair persists, failing later at purchase time in the scheduler. The swallow is documented as intentional in the function comment (:166-171) on the grounds that the frontend's hardcoded rules are the primary gate, which makes the backend check advisory on precisely the request it exists for. Returning 503 on a validation error, keeping only ErrNoData as the permissive case, would resolve it. Worth deciding alongside the permissive-versus-strict question in the commitmentopts.Validate bullet. Audit finding A02-022.

A05-012 (low)

The permissive-on-partial-data theme in the commitmentopts.Validate bullet extends into the Azure prober itself. probeCombo (internal/commitmentopts/probe_azure.go:188-196) can only return false from inside its loop, so an empty Benefits slice or a nil b.Valid pointer falls through to return true and the combo is recorded as sold. The 5-year candidate combos exist precisely so the API can reject what it does not sell, so a nil flag would persist P5Y combos as live and the frontend would offer a 5-year Azure savings plan that cannot be bought. probe_azure_test.go:138 and :150 pin that permissive behaviour as intended, so this is a deliberate choice worth revisiting rather than an oversight. Requiring at least one benefit with Valid == true would make it positive confirmation. Narrowing: ProbeAzure has no non-test caller today, so nothing currently reaches the store or the UI. Audit finding A05-012.

A05-013 (low)

Same function as the PlanID == "" item above, second defect. executableByScheduler also dereferences the plan with no nil guard: internal/purchase/manager.go:643 returns plan.AutoPurchase directly, while the sibling executePurchase explicitly handles GetPurchasePlan returning (nil, nil) at internal/purchase/execution.go:52-54 because the store interface permits it. A nil plan panics inside processOneExecution, and there is no recover() anywhere on the ProcessScheduledPurchases path (the only ones are internal/execution/fanout.go:115 and scheduler.go:1336), so one such row takes down the whole tick and every later due row goes unprocessed. Latent today because PostgresStore returns ErrNotFound, but the asymmetry with executePurchase is what makes it easy to trip on the next store or mock. Cheapest to fold into the same fix as the PlanID == "" short-circuit. (audit finding A05-013)

Acceptance criteria

• Each checklist item above is fixed (or explicitly re-triaged with a documented reason) with a regression test covering the described failure scenario.
• The cross-repo item (ladder plan Existing figure) is coordinated with the sibling issue in cloud-commitments-go so Allocate's net-existing value actually reaches assembleLadderPlan.
• The five 2026-09-02 audit findings (A16-005, A05-010, A02-022, A05-012, A05-013) are each closed out individually.