Repository navigation
fix(purchases): atomic ramp-step increment via SELECT FOR UPDATE - #1109
Merged
cristim merged 2 commits intoJun 8, 2026
Merged
Conversation
updatePlanProgress performed a read-modify-write on PurchasePlan.CurrentStep without locking: concurrent Lambda invocations could both read the same step, both increment it in memory, and both write CurrentStep+1 -- skipping a step or converging on the wrong index. Fix by adding IncrementPlanCurrentStep to StoreInterface and implementing it with a SELECT FOR UPDATE inside WithTx. The store locks the row before reading so only one caller at a time can advance the step counter. updatePlanProgress is simplified to a single delegate call; the step-advance logic now lives entirely inside the store method. All mock implementations updated across the api, analytics, purchase, scheduler, and server test packages. Closes #1071
Contributor
|
Warning Review limit reached
More reviews will be available in 23 minutes and 37 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR fixes a non-atomic read-modify-write race in plan progress tracking (issue ChangesPlan Progress Atomicity via Locked Increment
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
cristim
added
triaged
Claude
Member
Author
|
@coderabbitai review Generated by Claude Code |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and diff
gh pr view 1109 --repo LeanerCloud/CUDly
echo "=== FILES CHANGED ==="
gh pr diff 1109 --repo LeanerCloud/CUDlyLength of output: 24714 🏁 Script executed: #!/bin/bash
# Check WithTx implementation
rg -n "func.*WithTx" --type go -A 20
echo "=== RampSchedule IsComplete ==="
rg -n "IsComplete" --type go -A 5 -B 2
echo "=== UpdatePurchasePlanTx implementation ==="
rg -n "func.*UpdatePurchasePlanTx" --type go -A 20
echo "=== Store-level tests for IncrementPlanCurrentStep ==="
rg -rn "IncrementPlanCurrentStep" --type go
echo "=== pgxmock tests ==="
fd "store_postgres_test" --type f
fd "store.*test" --type f -e go | head -20Length of output: 36277 🏁 Script executed: #!/bin/bash
echo "=== pgxmock test file for IncrementPlanCurrentStep ==="
cat internal/config/store_postgres_pgxmock_test.go
echo "=== Check if any existing store test covers IncrementPlanCurrentStep ==="
grep -n "IncrementPlanCurrentStep\|CurrentStep\|increment\|ramp_step\|ramp.*step" internal/config/store_postgres_pgxmock_test.go 2>/dev/null || echo "NOT FOUND in pgxmock test"
grep -rn "IncrementPlanCurrentStep" internal/config/ 2>/dev/null
echo "=== purchasePlanSelectCols usage ==="
grep -n "purchasePlanSelectCols" internal/config/store_postgres.go
echo "=== scanPurchasePlanRow - check error context lost ==="
grep -n "scanPurchasePlanRow\|failed to get\|ErrNotFound" internal/config/store_postgres.go -A3 -B1
echo "=== current_step column check (is it separate or in JSON?) ==="
grep -rn "current_step" --type go | head -20Length of output: 80503
Bug:
|
| Test | What it covers |
|---|---|
TestPGXMock_IncrementPlanCurrentStep_AdvancesStep |
SELECT FOR UPDATE + CurrentStep++ + commit |
TestPGXMock_IncrementPlanCurrentStep_CompletedRamp |
CurrentStep already at TotalSteps, NextExecutionDate set to nil |
TestPGXMock_IncrementPlanCurrentStep_PlanNotFound |
pgx.ErrNoRows → nil (not an error) |
TestPGXMock_IncrementPlanCurrentStep_DBError |
query error → error propagated, rollback |
Minor: planID absent from lock-failure error message
return fmt.Errorf("failed to lock purchase plan: %w", err)Including planID here would make log triage faster:
return fmt.Errorf("failed to lock purchase plan %s: %w", planID, err)🧠 Learnings used
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.792Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), `POST /api/plans/{id}/purchases` (`createPurchaseExecutionsTx`) must stamp `created_by_user_id` via `resolveCreatorUserID(session)` at creation time. Without this, every scheduled purchase created via the Plans-page "Create planned purchases" action ships a NULL creator, which the ownership gate treats as a legacy row and denies management to all non-`update-any` users — including the creator themselves. The admin API key path must set `CreatedByUserID` to nil (not the key identity). Regressions: `TestHandler_createPlannedPurchases_StampsCreator` and `TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil`. Fixed in commit 9ef1b4352.
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.792Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:45:38.069Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/cloudsql/client.go`, `getSQLPricing` had the same per-hour/term-total unit mismatch as memorystore and cloudstorage: `commitmentPrice` (per-hour SKU rate) was passed directly to savings math that expected a term total, producing ~99.99% savings, and `HourlyRate` was set to `per-hour / hoursInTerm` (near-zero). Fixed in PR `#1047` commit 20590c6b4 (issue `#1078` folded in): `commitmentPriceTerm := commitmentPrice * hoursInTerm`; `HourlyRate = commitmentPrice` (raw per-hour). `convertGCPRecommendation` also hardcoded `rec.Term = "1yr"`, ignoring `params.Term`; fixed with same defaulting pattern as memorystore/cloudstorage. Regression tests: `TestGetSQLPricing_CommitmentPriceIsTermTotal` and `TestCloudSQLConvertGCPRecommendation_PropagatesTermFromParams`. PaymentOption was already correct (`if paymentOption == "" { paymentOption = "monthly" }`).
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai review Generated by Claude Code |
Contributor
|
I'll kick off a fresh review of the PR right away. 🧠 Learnings used✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/config/store_postgres.go (1)
514-514: 💤 Low valueConsider including
planIDin the lock-failure error message.Including the plan ID in the error (e.g.,
"failed to lock purchase plan %s: %w", planID, err) aids triage when multiple plans are processed concurrently.🔍 Proposed change
- return fmt.Errorf("failed to lock purchase plan: %w", err) + return fmt.Errorf("failed to lock purchase plan %s: %w", planID, err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/store_postgres.go` at line 514, The error returned from the lock operation currently uses fmt.Errorf("failed to lock purchase plan: %w", err) without the plan identifier; update the fmt.Errorf call to include the planID (e.g., fmt.Errorf("failed to lock purchase plan %s: %w", planID, err) or use %v if planID is not a string) so logs show which plan failed — change the fmt.Errorf expression at the lock-failure return site (the line that currently returns fmt.Errorf("failed to lock purchase plan: %w", err)) to include planID while preserving the %w error wrapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/store_postgres.go`:
- Around line 528-531: The plan's UpdatedAt must be refreshed before persisting:
after setting plan.LastExecutionDate (the block that sets now := time.Now() and
plan.LastExecutionDate = &now), also set plan.UpdatedAt = &now (or otherwise
assign time.Now()) so UpdatePurchasePlanTx receives a plan with a current
UpdatedAt; UpdatePurchasePlanTx only assigns UpdatedAt when it's zero, so
updating plan.UpdatedAt here (using the same now value) ensures the DB row is
written with a fresh updated_at timestamp.
---
Nitpick comments:
In `@internal/config/store_postgres.go`:
- Line 514: The error returned from the lock operation currently uses
fmt.Errorf("failed to lock purchase plan: %w", err) without the plan identifier;
update the fmt.Errorf call to include the planID (e.g., fmt.Errorf("failed to
lock purchase plan %s: %w", planID, err) or use %v if planID is not a string) so
logs show which plan failed — change the fmt.Errorf expression at the
lock-failure return site (the line that currently returns fmt.Errorf("failed to
lock purchase plan: %w", err)) to include planID while preserving the %w error
wrapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b62ffff-fa74-4c84-bf83-0ac2e72613b9
📒 Files selected for processing (14)
internal/analytics/collector_test.gointernal/api/mocks_test.gointernal/config/interfaces.gointernal/config/store_postgres.gointernal/mocks/stores.gointernal/purchase/approvals_test.gointernal/purchase/coverage_extra_test.gointernal/purchase/execution.gointernal/purchase/execution_test.gointernal/purchase/manager_test.gointernal/purchase/mocks_test.gointernal/purchase/money_path_regression_test.gointernal/scheduler/scheduler_test.gointernal/server/test_helpers_test.go
…c ramp step Address CodeRabbit findings on the atomic ramp-step increment (#1071): - Refresh plan.UpdatedAt inside IncrementPlanCurrentStep before persisting. The plan is read from the DB with its prior UpdatedAt, so UpdatePurchasePlanTx's zero-value guard would otherwise write a stale updated_at on every increment. - Include planID in the lock-failure error to aid triage under concurrency. - Add pgxmock regression tests covering the SELECT FOR UPDATE lock path, step advance + persisted updated_at, completed-ramp next_execution_date clearing, plan-deleted-mid-race tolerance, and lock-error rollback. The FOR UPDATE assertion fails if the row lock is dropped; the updated_at assertion fails without the refresh above.
Member
Author
CR round 1 addressedBoth findings from the full review were addressed in
Regression coverage addedThe atomic
Both fixes were verified load-bearing: removing Verification: |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
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
2 participants
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.
Summary
Closes #1071
updatePlanProgressperformed a non-atomic read-modify-write onPurchasePlan.CurrentStep: two overlapping Lambda invocations could both read the same step value, both increment it in memory, and both writeCurrentStep+1, skipping a step or landing the plan at the wrong index.IncrementPlanCurrentStep(ctx, planID string) errortoStoreInterface.store_postgres.gousingSELECT ... FOR UPDATEinsideWithTx. The lock prevents concurrent callers from reading stale step state; only the winner of the lock reads and writes the row.scanPurchasePlanRow(pgx.Row)as a shared helper soGetPurchasePlanandIncrementPlanCurrentStepuse the same scan logic.updatePlanProgressinexecution.goto a single delegate call; all step-advance logic now lives in the store method where the lock is held.api,analytics,purchase,scheduler, andservertest packages.Test plan
go test ./internal/config/...- passes (store-level logic)go test ./internal/purchase/...- allTestManager_UpdatePlanProgress*and integration tests passgo test ./internal/api/... ./internal/analytics/... ./internal/scheduler/... ./internal/server/scheduledauth/...- passgo test ./...- no new failures (two pre-existing failures ininternal/authandinternal/serverare unrelated)Generated by Claude Code
Summary by CodeRabbit
Tests
Refactor