Skip to content

test(purchase): prove the plan-step money path end to end and its crash points - #839

Merged
cristim merged 1 commit into
mainfrom
feat/631-pr2b2b-b4
Oct 13, 2026
Merged

cristim merged 1 commit into
mainfrom
feat/631-pr2b2b-b4

Conversation

@cristim

@cristim cristim commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

What (B4 of #631 PR2b-2b: proofs; tests only, no production change)

  • S9 (TestAzureStepTwo_ExecuteAndFinalize, mock provider, fixtures): step 2 after step-1 suppressions, priced from the unsuppressed stored row, buys; priced from the suppressed row it is refused before the provider (mutation: removing the stale-pricing call from executeAndFinalize fails 2 of 3 cases); a price change between notify and run is refused and the step status is failed.
  • S11 crash points: before commit nothing persists (real PG17, injected suppression failure); after commit before email: row stays pending and the next tick adopts it without resolving again (already in feat(purchase): attach resolved recommendations to a plan step under the provider lock #836); after email before notified: the injected rotation loss leaves the row pending, the next tick re-sends and promotes, one resolve (real PG17); after the provider call: TestRedriveReusesDerivedTokensForEveryProvider shows every recommendation gets DeriveIdempotencyToken(lineage, index) and a re-drive gets the identical tokens (aws rds, aws savings-plans, azure compute, gcp compute). The attach path persists recommendations sorted (B3), which is what keeps the index stable.
  • Per-provider token proof: see the matrix below. Issue Provider duplicate-purchase proof matrix: Azure Savings Plans has no server-side dedupe #838 tracks the gaps.

Provider proof matrix (pinned cloud-commitments-go, go.mod ad67030387e4)

Provider / service Token use Tested in the module
AWS Savings Plans ClientToken (savingsplans/client.go:247) savingsplans/client_test.go
AWS RDS, ElastiCache, MemoryDB, OpenSearch deterministic reservation ID + idempotency guard rds/client_test.go PurchaseCommitment_Idempotent_*
AWS EC2, Redshift tag lookup by token before buying ec2/redshift client tests; platform still refuses re-drive
Azure compute, cache, cosmosdb, database, managedredis, search, synapse DoIdempotentPurchaseTwoStep (token tag + reservation-order lookup) internal/reservations/purchase_test.go
GCP Compute Engine RequestId from the token + deterministic name (computeengine/client.go:931) computeengine/client_test.go:775-806

Double-buy risks (no provider-side proof): Azure Savings Plans (alias from UnixNano, no dedupe; the platform refuses re-drive, so only a lost-response first attempt is exposed). EC2/Redshift: guarded by the platform refusal, not a risk. GCP Cloud SQL, Cloud Storage and Memorystore cannot purchase. Any unknown provider is refused. Tracked in #838.

S14 DEV acceptance runbook (owner executes; nothing here turns anything on)

Do not run process_scheduled_purchases against real accounts. The kill switch stays OFF everywhere until you run this.

  1. DEV only: PUT /api/config with scheduling.auto_purchase=true; on one test plan set a small max_step_commitment and notification_days_before >= 1, and enable only the plan services you want resolved.
  2. Call /api/scheduled/send_notifications by hand (no Terraform schedule fires it; process_scheduled_purchases is scheduled only in cloudformation template.yaml:~728).
  3. Check the step row: status notified, recommendations sorted, scheduled_date at least notification_days_before days out, suppressions present, the email shows count, total commitment, cap and per-row details.
  4. CANCEL the purchase from the Plans page (or the email link) and confirm the suppressions are gone.
  5. Set scheduling.auto_purchase back to false.
    Prod: leave the kill switch off and caps NULL until you decide to flip them.

State

Kill switch OFF in every environment, caps NULL (refused). This PR changes no behavior. Linux parity: no new test compares database times; the attach fixtures already truncate to microseconds (#836). Remaining owner-listed items keep this at Refs #631.

Refs #631

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Expanded coverage for purchase-plan notifications when delivery is interrupted, including checks that retries do not repeat recommendation resolution.
    • Added verification of plan sizing based on eligible recommendations and safeguards against changed or suppressed pricing.
    • Added cross-provider checks for consistent retry behavior and refusal when a safe duplicate-prevention guard is unavailable.

…sh points

S9: an Azure step 2 priced from the unsuppressed stored row buys through
executeAndFinalize; the same step priced from the suppressed row, or repriced
between notify and run, is refused before the provider and the step fails.

S11: crash before commit persists nothing; crash after the email re-sends and
promotes without resolving again; after the provider call a re-drive reuses
DeriveIdempotencyToken(lineage, index) for every provider, and providers
without a duplicate guard are refused on re-drive.

Refs #631

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim cristim added priority/p1 Next up; this sprint severity/high Significant harm urgency/this-sprint Within the current sprint impact/many Affects most users effort/xl Multi-week / refactor type/feat New capability triaged Item has been triaged labels Oct 13, 2026
@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 749e8cec-47d3-4655-a938-296128991649
📥 Commits

Reviewing files that changed from the base of the PR and between 7a0dff4 and 2a8045d.

📒 Files selected for processing (2)
  • internal/purchase/plan_step_crash_integration_test.go
  • internal/purchase/plan_step_proofs_test.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

📝 Walkthrough

This change adds tests for notification failure recovery, Azure purchase sizing and pricing validation, and re-drive token reuse and refusal rules.

Changes: Notification recovery

Layer / File(s) Summary
Notification failure and retry tests
internal/purchase/plan_step_crash_integration_test.go
Tests check rollback when suppression creation fails before commit. They also check that a retry after token rotation failure sends email again and does not resolve recommendations a second time.

Purchase execution and re-drive

Layer / File(s) Summary
Azure sizing and pricing validation
internal/purchase/plan_step_proofs_test.go
Tests check Azure resolution from stored recommendation units. They also verify that suppressed or changed pricing fails with ErrStaleAzurePricing without provider calls.
Re-drive tokens and refusal rules
internal/purchase/plan_step_proofs_test.go
Tests check that re-drive reuses distinct tokens across AWS RDS, AWS Savings Plans, Azure compute, and GCP compute. They also test refusal rules for selected provider and service combinations.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to 2a804

The added purchase-flow tests are mergeable after normal checks.

Pre-merge checks · Passed 4 · Failed 1

Failed Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly identifies purchase-flow tests and crash-point coverage, which matches the main changes in the pull request.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@cristim

cristim commented Oct 13, 2026

Copy link
Copy Markdown
Member Author

Gate verdict (Opus 5.5 claude-opus-5-5, money path): APPROVE at 2a8045d

Independent adversarial review by a gate agent that did not write this PR. Diff is tests only (internal/purchase/plan_step_proofs_test.go, internal/purchase/plan_step_crash_integration_test.go); no production change.

Local evidence (macOS, git archive of the exact head, GOTOOLCHAIN=go1.26.9, throwaway local PostgreSQL 17 via an uncommitted test-helper override, under the suite lock):

  • go build ./..., go vet (with and without -tags integration) on internal/purchase: clean.
  • Unit: internal/purchase, internal/planstep, internal/config ok. New tests pass, -race -count=5 ok.
  • Integration: full internal/purchase with -tags integration ok; TestCrash_* and TestAttachIntegration_EmailFailure* -count=5: 15/15 pass.
  • git-secrets (--register-aws) over the tree: BSD grep rc=0, GNU grep rc=0; canary key detected.

Mutation run (one run, 9 mutants on the guarded production lines, all KILLED):

Mutant Killed by
drop staleAzurePricingRefusal from executeAndFinalize (manager.go:246) S9 suppressed-row + repriced cases
price from the unscaled row (planstep/resolve.go:236) S9 "resolved from the stored row buys"
token index fixed to 0 (execution.go:651) TestRedriveReusesDerivedTokensForEveryProvider (4 providers)
token from ExecutionID instead of lineage same
token not propagated to PurchaseOptions same
attach sort reversed (plan_step_attach.go:156) B3 attach tests (unit + integration)
Azure SP re-drive allowed (manager.go:~499) TestRedriveRefusedWhereNoProviderGuardExists + existing guard tests
suppression write error ignored (plan_step_attach.go:244) TestCrash_BeforeCommitPersistsNothing
lost token rotation treated as success (notifications.go:146) TestCrash_AfterEmailBeforeNotifiedResendsAndPromotes

Provider token trace in pinned cloud-commitments-go (ad67030387e4): AWS SP input.ClientToken providers/aws/services/savingsplans/client.go:247-248; RDS idempotencyGuard rds/client.go:150 and IdempotentReservationID :205; EC2 tag lookup ec2/client.go:139-140; Azure compute/cache/cosmosdb/database/managedredis/search/synapse all call reservations.DoIdempotentPurchaseTwoStep(..., opts.IdempotencyToken) (e.g. compute/client.go:508, managedredis/client.go:344; lookup in services/internal/reservations/purchase.go:647-657); GCP insertReq.RequestId = IdempotencyGUID(token) computeengine/client.go:931-932, tested client_test.go:778-810. Issue #838 is open and lists the gaps.

Runbook (S14): DEV only, small cap, notification_days_before >= 1, manual send_notifications, CANCEL, kill switch back off, never process_scheduled_purchases on real accounts, prod untouched. OK.

Linux parity: token capture uses a mutex and map equality (no order dependence); integration tests compare no DB timestamps. CI green on this SHA, including Integration Tests on Linux.

Non-blocking note (docs accuracy, no code risk): the PR body and #838 say Azure Savings Plans uses a UnixNano alias with no dedupe. In the pinned module, savingsplans/client.go:373 derives the alias from the token via common.ReservationOrderID(opts.IdempotencyToken, fallback) (pkg/common/tokens.go:115); UnixNano is only the empty-token (CLI) fallback. Whether Azure treats a repeated alias PUT as idempotent is not proven in the module, so keeping the platform re-drive refusal is still the safe call; the comment at manager.go:~496 ("timestamp-based alias name") and #838 should be updated.

Merge-result check: main advanced to 7657a38 (#837, ladder, touches internal/purchase) after this head; GitHub reports CLEAN. The would-be merge tree (fa08e58b) vets with and without -tags integration, and internal/purchase unit + integration tests pass on it locally.

Merging with --squash --match-head-commit 2a8045de58365254e16bcc1c1fc08b8c3ef002f5.

@cristim
cristim merged commit 4cecfdb into main Oct 13, 2026
26 checks passed
@cristim
cristim deleted the feat/631-pr2b2b-b4 branch October 16, 2026 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xl Multi-week / refactor impact/many Affects most users priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/feat New capability urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant