Repository navigation
Conversation
The adapter unit test's ReserveRIExchange mock ignored the daily and per-exchange cap strings, so swapping them in the adapter still passed. The mock now records both and the test asserts each one. The real-Postgres integration test set the standalone daily cap to baseline + 1000 through a float, equal to the per-exchange cap at a zero baseline, so either cap could bind, and a fractional-micro baseline floored the cap to 999.999999. Caps are now today's spend rounded up to whole dollars plus a whole-dollar margin: the standalone run leaves 1500 USD of daily headroom (per-exchange cap binds at 1000), and the ladder run leaves 980 USD after the standalone exchange (daily cap binds), with the expected ceiling computed exactly from the spend. Whether a cap-refused reservation should write a failed audit row is left to the owner (it is pkg behavior in cloud-commitments-go). Refs LeanerCloud#666 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
impact/internal
Contributor
|
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 |
This branch has not been deployed
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
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.
Tightens the two RI exchange reservation tests flagged as non-blocking in the independent review of #662. The audit-row question (item 3) is left for the owner; the options are below. Test-only change: no production code changed.
Root cause and fix
Adapter unit test ignored the cap arguments. In
TestConfigExchangeStoreAdapter, theReserveRIExchangemock droppeddailyCapUSD/perExchangeCapUSD, so swapping them inconfigExchangeStoreAdapter.ReserveRIExchangestill passed. The mock now records both, and the subtest asserts"20"reached the store as the daily cap and"10"as the per-exchange cap. (internal/server/handler_ri_exchange_test.go)Integration test caps could tie, or flake by one micro.
TestRIExchangeAutoWorkers_ConcreteStoreAndFixtureClientset the standalone daily cap tobaseline + 1000through a float64:floorCapUSDfloored1000.3to1000.299999, which left999.999999of headroom and broke the"1000.000000"assertions.Now each cap is today's spend rounded up to whole dollars, plus a whole-dollar margin. A float64 holding an integer converts to the six-decimal cap string exactly. The two runs use distinct caps, so a different cap binds in each:
1000.000000).950.000000. This leaves 980 to 981 USD of headroom, so the daily cap must bind. The expected ceiling is computed exactly as daily cap minus spend, usingbig.Rat.The fixture
Executechecks each run's expected ceiling twice: againstMaxPaymentDueUSDand against the reserved row'spayment_due.Item 3 (owner decision, not decided here): audit row for cap-refused reservations
Today (pkg
exchange/auto.go,processAutoExchange), ifReserveRIExchangereturns an error, nothing is written tori_exchange_history. That covers cap refusals (ErrRIExchangeDailyCapExceeded) and store errors. The refusal still shows up inAutoExchangeResult.Failed, in the run log line, and in the "completed" notification email (as the item'sError). Failed rows don't count toward daily spend, which counts onlyprocessingrows and today'scompletedrows, so every option below is spend-neutral.failedrow for every reservation error in pkg: callsaveFailedRecordin theReserveRIExchangeerror branch. This needs a cloud-commitments-go change, a pkg bump in this repo'sgo.mod, and flipping the pkg unit test that now expects 0 failed rows. Downsides: repeated rows on every run while the cap stays exhausted, and on a DB outage the extra insert will usually fail too.ErrRIExchangeDailyCapExceededlives in this repo'sinternal/config) or in this repo'sconfigExchangeStoreAdapter.ReserveRIExchangeonerrors.Is(err, config.ErrRIExchangeDailyCapExceeded). The adapter route needs no pkg change. A variant uses a distinct status such asrefusedrather thanfailed, which needs a migration ifstatushas a CHECK constraint.Once a direction is picked, it can go in a follow-up PR (or a new commit here).
Refs #666
Verification
The unit evidence uses a mock store. The integration evidence uses the existing real-Postgres test (a testcontainers
postgres:16-alpineinstance with all migrations applied) and a fixture exchange client. No AWS calls were made.Commands (with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true):All pass on this head. gocyclo doesn't apply because no non-test file changed.
Mutation probes. Each one was applied temporarily to production code, run against the old tests (origin/main) and the new tests, then reverted:
ReserveRIExchange(ctx, cfgRecord, perExchangeCapUSD, dailyCapUSD)), unit testExpected 20, got 10return riExchangeUSD(perCap))expected "980.000000",actual "1000.000000"remaining := dailyCap)expected "980.000000",actual "1000.000000"minwithperCap)expected "1000.000000",actual "1500.000000"0.300000USD inserted before the baseline read)actual "999.999999")Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.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:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns 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: 137 PASS, 0 FAIL.Notes and out of scope
internal/server/handler_ri_exchange.goisn't touched.950.000000spend-delta assertion fails. The window is a few milliseconds. The old test depended on the same day boundary, less strictly.riExchangeClientslines 77-79, fix(ri-exchange): exclude convertible RIs with no pricing from cost math #1191 edits theri()fixture line 65, and fix(db): let ri_exchange_history.account_id hold Azure and GCP identifiers #1170 edits theReserveRIExchangesubtest'srecordline. This PR avoids those exact lines.quoteClient's signature is unchanged, and the per-run expected ceilings come from a map keyed by RI ID.Maintainers: please mirror the issue's triage labels onto this PR:
impact/internal,type/chore,priority/p3,severity/low,urgency/eventually,effort/s.🤖 Generated with Claude Code