fix: prevent orphaned orders when refund occurs during OrderCreated validation #642
Conversation
…alidation - Update existing orders with gatewayID before validation to ensure they exist in DB - Pass existing order through validation chain to HandleCancellation - Update createBasicPaymentOrderAndCancel to update existing orders instead of creating duplicates - Fixes race condition where orders get refunded during validation but remain orphaned at 'initiated' status
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughWhen API-created orders lacking a gatewayID already exist in the system, the code now updates the existing record with gatewayID, transaction hash, and block number before validation, then reloads and passes this updated order through subsequent processing paths. Function signatures extended to accept optional existing orders, influencing cancellation and validation logic. Changes
Sequence DiagramsequenceDiagram
actor Client
participant OrderService as Order Service
participant DB as Database
participant Validator as Validator
Client->>OrderService: Trigger order processing
activate OrderService
OrderService->>DB: Check for existing order by message hash
alt Order exists but lacks gatewayID
DB-->>OrderService: Return existing order
OrderService->>OrderService: Update order with gatewayID,<br/>tx hash, block number
OrderService->>DB: Persist updates
DB-->>OrderService: Confirmation
OrderService->>DB: Reload updated order
DB-->>OrderService: Return refreshed order
rect rgb(200, 240, 255)
note over OrderService,Validator: Pass updated order to validation
OrderService->>Validator: Validate with existing order context
end
else Order doesn't exist or has gatewayID
rect rgb(240, 240, 240)
note over OrderService,Validator: Standard validation path
OrderService->>Validator: Validate order data
end
end
Validator-->>OrderService: Validation result
OrderService->>OrderService: Process cancellation if needed
OrderService-->>Client: Complete
deactivate OrderService
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/common/order.go (1)
1134-1160: Potential issue: Inconsistent handling may create duplicate orders.Line 1117 correctly passes
existingOrdertoHandleCancellationto update the existing order:err := HandleCancellation(ctx, existingOrder, nil, "Amount is less than the minimum bucket", refundOrder)However, the rate validation cancellations at lines 1135, 1143, and 1155 still pass
nil, paymentOrderFields:err := HandleCancellation(ctx, nil, paymentOrderFields, "Rate validation failed", refundOrder)If
existingOrderWithMessageHashwas present and updated withgatewayIDat lines 94-100, callingHandleCancellationwithpaymentOrderFieldswill create a new cancelled order with the samegatewayID, potentially resulting in duplicates.Consider applying the same pattern consistently:
🔎 Suggested fix
if rateResult.Rate == decimal.NewFromInt(1) && paymentOrderFields.Rate != decimal.NewFromInt(1) { - err := HandleCancellation(ctx, nil, paymentOrderFields, "Rate validation failed", refundOrder) + err := HandleCancellation(ctx, existingOrder, paymentOrderFields, "Rate validation failed", refundOrder) if err != nil { return nil, nil, nil, nil, nil, fmt.Errorf("failed to handle cancellation: %w", err) } return nil, nil, nil, nil, nil, nil } if rateErr != nil { - err := HandleCancellation(ctx, nil, paymentOrderFields, fmt.Sprintf("Rate validation failed: %s", rateErr.Error()), refundOrder) + err := HandleCancellation(ctx, existingOrder, paymentOrderFields, fmt.Sprintf("Rate validation failed: %s", rateErr.Error()), refundOrder) if err != nil { return nil, nil, nil, nil, nil, fmt.Errorf("failed to handle cancellation: %w", err) } return nil, nil, nil, nil, nil, nil } // Check if event rate is within 0.1% tolerance of validated rate tolerance := rateResult.Rate.Mul(decimal.NewFromFloat(0.001)) // 0.1% tolerance rateDiff := event.Rate.Sub(rateResult.Rate).Abs() if rateDiff.GreaterThan(tolerance) { - err := HandleCancellation(ctx, nil, paymentOrderFields, "Rate validation failed", refundOrder) + err := HandleCancellation(ctx, existingOrder, paymentOrderFields, "Rate validation failed", refundOrder) if err != nil { return nil, nil, nil, nil, nil, fmt.Errorf("failed to handle cancellation: %w", err) } return nil, nil, nil, nil, nil, nil }Note:
HandleCancellationcurrently treatscreatedPaymentOrderandpaymentOrderFieldsas mutually exclusive (line 629). If both are provided, it returns early. This may need adjustment if you want to support updating an existing order with additional fields frompaymentOrderFields.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/common/order.go
🧰 Additional context used
🧬 Code graph analysis (1)
services/common/order.go (2)
ent/paymentorder.go (2)
PaymentOrder(24-107)PaymentOrder(204-235)types/types.go (1)
OrderCreatedEvent(71-81)
🔇 Additional comments (5)
services/common/order.go (5)
90-113: LGTM - Core fix for the race condition.Updating the order with
gatewayIDbefore validation ensures the order exists in the DB with the gateway identifier before any refund processing can occur. The reload after update correctly fetches the updated entity state.One edge case to verify: if the update at line 94-100 succeeds but subsequent validation fails and the entire
ProcessPaymentOrderFromBlockchainreturns an error, the order will remain withgatewayIDset but potentially incomplete data. This appears intentional given the PR objective, but ensure downstream code handles this partial state correctly.
116-117: LGTM - Correctly propagates existing order through validation.Passing the existing order enables proper cancellation handling without creating duplicates.
1014-1023: LGTM - Function signature update is well-documented.The added parameter and comment clearly explain the purpose of the optional existing order.
1056-1056: LGTM - Existing order correctly passed for early validation failures.These changes ensure that if an existing order was updated with
gatewayIDbefore validation, the cancellation path will update that order instead of creating a duplicate.
1225-1236: LGTM - Function signature and documentation updated appropriately.The comment clearly explains the conditional behavior based on whether
existingOrderis provided.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Description
This PR fixes a race condition where orders become orphaned when refunds occur during OrderCreated event validation.
Problem:
When an OrderCreated event is indexed and
ProcessPaymentOrderFromBlockchainis called:messageHashbut nogatewayID(status="initiated") is foundvalidateAndPreparePaymentOrderDatais called BEFORE updating the order in the databasecreateBasicPaymentOrderAndCancelis calledHandleCancellationcreates a NEW order withgatewayIDand status "cancelled"gatewayIDgatewayIDSolution:
gatewayIDBEFORE validation: If an order exists withmessageHashbut nogatewayID, update it immediately withgatewayID,txHash, andblockNumberbefore calling validationvalidateAndPreparePaymentOrderDataand through tocreateBasicPaymentOrderAndCancelcreateBasicPaymentOrderAndCancelto check if an order with the matchinggatewayIDexists, and if so, pass it toHandleCancellationwithpaymentOrderFields = nilso it updates the existing order instead of creating a duplicateImpact:
gatewayIDin the database before any refund processingReferences
N/A - This is a bug fix for an internal race condition issue.
Testing
Manual Testing Steps:
messageHash, nogatewayID)gatewayIDsetEnvironment:
Go 1.25.0
PostgreSQL (via Ent ORM)
Redis
This change adds test coverage for new/changed/fixed functionality
Checklist
main(base:stable)By submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.