chore(purchase/ladder/exchange): medium/low findings from the 2026-07-28 full review #1583

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

Tracking issue for the MEDIUM and LOW findings from the 2026-07-28 money-core review of internal/purchase/*, internal/commitmentopts/*, pkg/ladder/*, pkg/exchange/* and their immediate spend-deciding callers. The CRITICAL and HIGH findings from the same pass are filed as individual issues.

One finding from this pass, buy-now ramp steps escaping netting, is already tracked by LeanerCloud/cloud-commitments-go#36 and is not repeated here; the new evidence was added as a comment there. It was re-scored HIGH on this pass (see the note at the bottom of this issue).

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 feat(purchases): flip RecoverStrandedApprovals to idempotent re-drive (now that #636 makes it safe) #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(purchase): retrying a per-account fan-out row re-fans-out and double-buys every account that already succeeded #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(purchases): SQS + cron executor paths lack atomic status guard before purchase (double-execute) #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.

• capOrUnlimited turns an unset money cap into math.MaxFloat64 on the auto-executing reshape path (MEDIUM)

  • Where: providers/aws/ladder/reshape.go:23 (unlimitedCapUSD = math.MaxFloat64), :143-155 (capOrUnlimited), :110-138 (buildRIExchangeConfig hard-codes Mode: auto)
  • What: ladder.BufferReshapeConfig.MaxPaymentPerExchangeUSD / .MaxPaymentDailyUSD are *float64, so their zero value is nil, and nil is mapped to "no cap" while Mode is unconditionally auto (exchanges execute immediately). The rejection logic is inverted relative to risk: an explicit 0 is rejected loudly, an omitted value is accepted as infinite.
  • Failure scenario: L16 wires the ladder reshape executor and builds BufferReshapeConfig{UtilizationThresholdPct: 80, LookbackDays: 30, LadderRunID: &id}, forgetting the two optional caps. buildRIExchangeConfig returns MaxPaymentPerExchangeUSD = MaxPaymentDailyUSD = 1.8e308, RunAutoExchange runs in auto mode, chooseEffectiveCap returns an effectively infinite bound, and every under-utilized convertible RI is exchanged at whatever AWS quotes, with no ceiling. Currently latent: git grep "BufferReshapeConfig{" finds only test constructions, and ReshapeBuffer fails loud with errWriteNotWired.
  • Fix: require both caps on the reshape path (reject nil the way 0 is rejected), or gate "no cap" behind an explicit Unlimited bool field, so omission can never mean infinite spend.

• pkg/exchange treats any mode string other than "manual" as auto-execute (MEDIUM)

  • Where: pkg/exchange/auto.go:248 (if params.Config.Mode == "manual" { ... } else fall through to processAutoExchange), with the typed constants ExchangeModeAuto / ExchangeModeManual defined but unused at this branch (auto.go:603-608)
  • What: the single most consequential branch in the package, approve-first vs spend-now, is a bare string comparison whose default is "spend". An empty, misspelled, or future mode value routes to real, irreversible AcceptReservedInstancesExchangeQuote calls. This is the inverse of the repo's fail-closed money rule and of the typed-enum rule (feedback_prefer_typed_enums).
  • Failure scenario: RIExchangeConfig.Mode is populated from cfg.RIExchangeMode (internal/server/handler_ri_exchange.go:102) and from buildRIExchangeConfig on the ladder path. Today the API write boundary validates manual|auto (internal/api/handler_ri_exchange.go:1498-1501) and the store defaults "" to "manual" (internal/config/store_postgres.go:251-253), so this is defence-in-depth. But pkg/ is a separate public Go module with a struct literal any caller can build: a caller that leaves Mode unset (its zero value) gets automatic execution instead of an approval-gated pending record.
  • Fix: switch on the typed ExchangeMode and return an error on any value that is not exactly ExchangeModeAuto or ExchangeModeManual.

• Failed pending-exchange cancellation is logged and ignored, leaving two live approval tokens for the same RI (MEDIUM)

  • Where: pkg/exchange/auto.go:167-178 (CancelPendingExchangesByOrigin error becomes logging.Warnf and the run continues), then :373-402 mints a new record plus token for the same source RI; expiry at :371
  • What: step 1 of RunAutoExchange is the only mechanism that retires the previous run's pending records; its failure is non-fatal and the run proceeds to create fresh ones. Each pending record carries its own ApprovalToken and a 24h ExpiresAt, while the email tells the user the window is 6h because the next run's cancellation is expected to close it.
  • Failure scenario: a transient DB error makes CancelPendingExchangesByOrigin return an error at 06:00. The run proceeds and writes a second pending record for RI-A with a second live token. Both approval emails are valid for up to 24h. An operator clicks the older link (larger stale payment_due); the approval handler's own atomic status guard prevents a double exchange, but the exchange that executes is the one quoted against stale pricing, and the daily-spend ledger is charged the amount from whichever record won, not the amount the operator was shown in the newer email.
  • Fix: treat a cancellation failure as fatal for the run (return before creating new pending records), or scope new records so an un-cancelled predecessor for the same source RI blocks minting a second token. Adjacent to ladder: pending-exchange cancellation is scoped is-ladder, not per-run/per-config cloud-commitments-go#32 and feat(exchange): add true simulation mode to RunAutoExchange; make pending-cancellation coordination explicit cloud-commitments-go#29, which cover cancellation scoping rather than cancellation failure handling.

• 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())) (providers/azure/services/savingsplans/client.go:282), the deterministic derivation landed for Azure Savings Plan purchase is non-idempotent (double-buy on re-drive) #1079 in PR sec(azure): harden HTTP clients + fix fabricated reservation prices in managedredis/savingsplans and 5 pricing clients #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 (providers/azure/recommendations.go:229-231). The moment that stub is implemented (tracked by feat(azure/savings-plans): wire SP rec collection into scheduler cloud-commitments-platform#17) 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 sec(azure): harden HTTP clients + fix fabricated reservation prices in managedredis/savingsplans and 5 pricing clients #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: Normalize 'savings-plans' vs 'savingsplans' identifier across frontend + backend cloud-commitments-platform#7 (normalising the savings-plans / savingsplans identifier repo-wide) and Azure Savings Plan purchase is non-idempotent (double-buy on re-drive) #1079.

• 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" (pkg/common/types.go:125) and common.ServiceCompute = "compute" (pkg/common/types.go:73); 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 feat(history): Sell-on-Marketplace button for Standard RIs (CreateReservedInstancesListing) with status polling + sell-{any,own} RBAC #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.

• Ladder plan reports Existing gross of expiring commitment, inconsistent with the E used to size the gap (LOW)

  • Where: internal/server/handler_ladder.go:575-582 (existingTotal += *ls.ExistingUSDPerHour) vs pkg/ladder/allocate.go:387-409 (computeNetExisting returns existing - expiring)
  • What: Allocate sizes the gap against net existing commitment; assembleLadderPlan 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: report the same net figure computeNetExisting produced, threading it out of Allocate rather than recomputing from layerStates.

• 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)

  • 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.

Note on LeanerCloud/cloud-commitments-go#36 (buy-now ramp steps escape netting)

That finding was re-scored HIGH on this pass rather than MEDIUM, and the evidence is now on LeanerCloud/cloud-commitments-go#36 as a comment. The reason for the escalation is that it is an approval-integrity problem, not only an accounting one: allocTotalHourlyCommit (handler_ladder.go:645-648) and the ActionPurchase entries in assembleLadderPlan (:558-568) both report the full allocation gap into the plan JSON and ladder_runs.total_hourly_commit, while persistLadderRun (:437-465) persists only the delayed tranches. With a ramp whose first step is AfterDays: 0, the approver signs off on a number that is roughly 2x what the run will actually place. LeanerCloud/cloud-commitments-go#36 currently carries priority/p2 / severity/medium; it is worth re-labelling to severity/high given that the operator-facing approval figure is the thing that is wrong.

Findings from the 2026-09-02 codebase audit

Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.

A16-005 (high)

The ctx bullet on this function 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 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 existing bullet. Audit finding A02-022.

A05-012 (low)

The permissive-on-partial-data theme in the commitmentopts 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, 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)