Repository navigation
Conversation
…nding them up Tranche amounts are float64 stored in NUMERIC(20,6). Two seams could round a planned commitment up, against the go precision contract (pkg/ladder LadderCapability.PurchaseLayer, go#354/LeanerCloud#403/LeanerCloud#438: floor, never round up; refuse outside [0.001, 1000000] USD/hr; truncate to five decimals): - buildTrancheDBRows converted the exact *big.Rat to the nearest float64. - Postgres rounded the float's decimal half-up to six places on insert, so 0.0009995 was stored as the 0.001 minimum, 999999.9999999 as 1000000 and 0.1234599 as 0.123460 (five-decimal wire 0.12346 instead of 0.12345). The planner now floors to six decimals (config.LadderAmountFloat), and the store writes an exact decimal and refuses an amount the column would round (ladderAmountText), so every stored amount reads back as the same float64. No migration: six decimals already exceed the provider's five. TestLadderB1_DailyRerunNeverExceedsTarget now requires live <= target on every day (half-up rounding over-planned to 10.000001 on day 7) and allows the floored fixed point to sit up to one micro-dollar per ramp step below it. Refs LeanerCloud#1182 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
|
Independent review 2 (claude-fleet, cold): APPROVE f1bdefa Findings (none blocking):
Against #1182 item 1: both seams are fixed.
Attacked:
Verified locally, with no cloud credentials, under the heavy lock, with testcontainers Postgres:
Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged. |
|
VERDICT: REQUEST_CHANGES f1bdefa Rebase check: Findings
Checked and correct
Attacked
|
Address the LeanerCloud#1199 review. A positive planned amount below 0.000001 USD/hr floors to 0 at the six-decimal tranche column; buildTrancheDBRows now drops that tranche with a log line instead of persisting a $0 scheduled row, and refuses a zero or negative planned amount outright. immediateTrancheAmounts sums the same floored amounts, so plan_json never shows more than the rows netting reads. Document that the floored float can round up only above about 1e9 USD/hr. Refs LeanerCloud#1182 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the reviews of f1bdefa. New head: 606ecaf. This merges origin/main (c8bdd46, clean) and adds one fix commit. VERDICT: REQUEST_CHANGES
Independent review 2: APPROVE
PR body: describes the drop/refuse behavior and the new tests, notes the 0.001 plan-time question as open, and replaces the stale "behind main" note. Verification on the merged tree (no cloud credentials,
|
Root cause
Item 1 of #1182: tranche amounts are float64 stored in
ladder_tranches.amount_usd_hr NUMERIC(20,6). I checked them against the go precision contract at the version go.mod pins (pkg/ladder/capability.goPurchaseLayerdoc from go#438, andproviders/aws/internal/spcommitment.Normalizefrom go#403). That contract says to floor the commitment, never round it up, refuse anything outside [0.001, 1000000] USD/hr, and truncate the shortest decimal to 5 places.Any amount whose decimal fits in 6 places already round-tripped exactly. pgx sends the float's shortest decimal and reads it back with
ParseFloat. That covers every 5-decimal contract value, the range ends, and values float64 can't represent exactly such as 0.1, 0.29 and 999999.99999. Two seams could still round a planned amount up:buildTrancheDBRowsconverted the exact planned*big.Ratto the nearest float64. The contract says "callers converting a *big.Rat amount to float64 must not round it up". For example, 0.12345999999999999 became 0.12346.On insert, Postgres rounded the float's decimal half-up to 6 places:
Planner amounts are routinely finer than 6 places (1/3, ramp halvings), so this was a real path, not just a theoretical one.
Fix (no migration)
config.LadderAmountFloatfloors the exact planned amount to the column's 6 places. Flooring at 6 and then truncating at 5 gives the same result as truncating the exact amount at 5. An amount below a minimum also stays below it.buildTrancheDBRowsnow uses it.buildTrancheDBRowsdrops that tranche with a log line instead of writing a $0 scheduled row; such an amount is far below the provider's 0.001 minimum and could never be bought, and refusing it would fail the same run on every rerun. A zero or negative planned amount (whichBuildTranchesnever emits) is refused, failing the run.immediateTrancheAmounts) sums the same floored amounts, soplan_jsonnever shows more than the persisted rows.insertLadderTranchesTxwrites an exact decimal ($5::numeric) throughladderAmountText. That helper refuses a non-finite amount, or one with more than 6 decimals that the column would round, so every stored amount reads back as the same float64. Planner rows are already floored, so they never hit the refusal.Item 2 (refuse tranches without a CommitmentRate): left for #118
There is still no tranche executor on main.
ClaimDueLadderTranchesexists but has no production caller. It deliberately passes legacy NULL-basis rows andon_demand_equivalentrows (both without a rate) to the callback, andTestLadderTrancheProvenance_InsertClaimAndReconcilepins that behavior. Whether the executor should fail the batch, skip those rows, or cancel them with a status_reason is a design choice for #118, so I didn't add a guard here.Design constraint for #118: the executor must refuse any claimed row whose
CommitmentRateis nil rather than defaulting the rate to 1. New planner rows always carryamount_basis='commitment'plus a rate.insertLadderTranchesTxand theladder_tranche_amount_provenanceCHECK already reject a commitment-basis row without a rate.Verification
Run on macOS against real Postgres (testcontainers,
postgresimage) via the existing integration harness. Tests ran withAWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/nullbecause this machine's default AWS profile is invalid. All evidence is unit tests plus local real-Postgres integration tests; nothing calls a real cloud API.New tests:
internal/config/store_postgres_ladder_precision_integration_test.goTestLadderTrancheAmount_RoundTripsProviderPrecisionContract. It stores contract edges (0.001, 0.000999, 0.00101, 0.1, 0.29, 0.12345, 1.00001, 123456.78901, 999999.99999, 999999.999999, 1000000, 1000000.000001, 99999999.999999) and floored planner rats (1/3, 0.12345999999999999, 0.1234567, 0.0009995, 999999.9999999, 1000000). It reads them back throughClaimDueLadderTranches(the float scan an executor will use) and checks three things:amount_usd_hr::textholds the exact decimal;spcommitment.Normalize) are the same for the stored amount as for the written one, or for the exact plan in the planner cases.Floats the column would round (0.1234599, 0.0009995, 999999.9999999, 1/3, NaN) must be refused, with the batch rolled back.
internal/config/ladder_provenance_test.go: unit tests forLadderAmountFloatandladderAmountText.internal/server/handler_ladder_test.goTestBuildTrancheDBRows_FloorsAmountToStoredScale,TestBuildTrancheDBRows_DropsAmountFlooringToZero,TestBuildTrancheDBRows_NonPositiveAmountErrorsandTestImmediateTrancheAmounts_SumsStoredAmounts.TestLadderB1_DailyRerunNeverExceedsTarget(existing test, tightened): live commitment must now be<= targeton every one of the 40 days. Before, the bound wastarget+1e-6, which tolerated the half-up overshoot. The final fixed point may now sit up to 1e-6 per ramp step below the target, never above it. That is far under the provider's 1e-5 precision.Fail-before / pass-after (mutation runs against the pre-fix code, with the new tests kept):
After the fix, all of these pass:
Concurrency: this only changes how one row's value is encoded inside the existing insert statement. No new locking, reads or write ordering, so there is no new race to test.
Out of scope / notes
PurchaseLayer.ladder_runs.total_hourly_commit(a dashboard total, the float sum of the rows) is still rounded half-up by its own NUMERIC(20,6). It is not a purchase input, so I left it alone.Maintainer: please mirror the issue's triage labels onto this PR:
severity/medium,triaged,priority/p3,urgency/this-quarter,impact/few,effort/s,type/bug.Refs #1182
🤖 Generated with Claude Code