-
Notifications
You must be signed in to change notification settings - Fork 20
feat: Implement provider attempt tracking to prevent immediate exclusion on assignment failure #650
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add attempt count tracking for providers per order in Redis - Only exclude providers after 3 failed attempts instead of immediate exclusion - Track attempts when order_request expires (provider didn't accept/fulfill) - Use refund timeout for attempt tracking TTL (OTC vs regular orders) - Move attempt count check to after rate/balance validation but before assignment - Remove immediate exclusion on assignment failure to allow retries
📝 WalkthroughWalkthroughImplements provider attempt tracking for order reassignment. The system tracks provider assignment attempts per order using Redis, incrementing counts when order requests expire. During reassignment, providers with 3+ attempts are excluded. TTLs differ for OTC versus regular orders. Changes
Sequence DiagramsequenceDiagram
participant OrderTask as Order Reassignment Task
participant PQService as PriorityQueueService
participant Redis as Redis Cache
participant DB as Database
OrderTask->>PQService: Initial AssignPaymentOrder(orderID, providerID)
PQService->>Redis: Check order_exclude_list_<orderID>
Redis-->>PQService: Not in exclude list
PQService->>DB: Assign provider
DB-->>PQService: Success
Note over OrderTask: Order request expires<br/>(provider didn't accept)
OrderTask->>PQService: ReassignStaleOrderRequest
OrderTask->>PQService: IncrementProviderAttemptCount(orderID, providerID, orderType)
PQService->>Redis: HINCRBY order_provider_attempts_<orderID> providerID 1
Redis-->>PQService: Attempt count = 3
PQService->>Redis: Set TTL on attempt tracking (based on orderType)
Redis-->>PQService: TTL updated
OrderTask->>PQService: matchRate (reassignment)
PQService->>Redis: getProviderAttemptCount(orderID, providerID)
Redis-->>PQService: Attempt count = 3
PQService->>Redis: Add to order_exclude_list_<orderID>
Redis-->>PQService: Excluded
PQService-->>OrderTask: Retry with next provider
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tasks/tasks.go (1)
1294-1342: Potential bypass of attempt count check when ProviderID is set.When
ReassignStaleOrderRequestcallsAssignPaymentOrderwithProviderIDset (line 1328), the attempt count check inmatchRateis bypassed. InAssignPaymentOrder(lines 437-500 inpriority_queue.go), ifProviderIDis set and not in the exclude list, the code directly callsassignOtcOrderorsendOrderRequestwithout checking the provider's attempt count.This means a provider could be retried indefinitely as long as they're not added to the exclude list, defeating the purpose of the 3-attempt limit for orders with a preferred provider.
Consider either:
- Adding attempt count check before lines 472-476 and 479-482 in
priority_queue.go, or- Setting
ProviderIDto empty string after incrementing attempts, so the order goes throughmatchRatewhich has the attempt checkOption 2: Clear ProviderID after incrementing attempts
err = priorityQueueService.IncrementProviderAttemptCount(ctx, order.ID.String(), providerID, orderType) if err != nil { logger.WithFields(logger.Fields{ "Error": fmt.Sprintf("%v", err), "OrderID": order.ID.String(), "ProviderID": providerID, }).Warnf("Failed to increment provider attempt count") // Continue anyway - not critical } + // Clear provider ID so order goes through matchRate which checks attempt counts + providerID = "" }
🧹 Nitpick comments (1)
services/priority_queue.go (1)
758-781: Provider tracking fallback consideration.The provider is set in the database for attempt tracking when the order request expires. If this fails (logged as warning), the
providerIdis still available in Redis (line 836), butReassignStaleOrderRequestintasks/tasks.goretrieves the provider from the database relation (order.Edges.Provider), not from the Redis hash.Consider whether the Redis
providerIdfield should be used as a fallback inReassignStaleOrderRequestwhenorder.Edges.Provideris nil, to ensure attempts are tracked even if the database update fails.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
services/priority_queue.gotasks/tasks.go
🧰 Additional context used
🧬 Code graph analysis (2)
tasks/tasks.go (2)
services/priority_queue.go (1)
NewPriorityQueueService(39-43)ent/paymentorder/paymentorder.go (2)
OrderType(313-313)OrderTypeOtc(320-320)
services/priority_queue.go (2)
storage/redis.go (1)
RedisClient(13-13)utils/logger/logger.go (4)
WithFields(76-109)Fields(73-73)Infof(125-130)Warnf(133-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (7)
services/priority_queue.go (6)
7-7: LGTM!The new imports for
strconv(parsing attempt counts) andredis(checkingredis.Nilerror) are correctly added and necessary for the new attempt tracking functionality.Also applies to: 25-25
674-700: LGTM!The implementation correctly uses atomic
HIncrByfor concurrent-safe increment, applies appropriate TTLs based on order type, and refreshes TTL on each increment as specified in the requirements. The logging provides good observability.
702-719: LGTM!The method correctly handles the
redis.Nilcase by returning 0 (provider hasn't been attempted yet), which is the expected behavior for the attempt tracking logic.
595-598: LGTM!Adding
providerIdto the Redis order request data enables tracking which provider was assigned when the order request expires, which is essential for the attempt tracking feature.
1062-1092: LGTM!The attempt count check for OTC orders is correctly placed after rate validation but before assignment, as specified in the requirements. The fail-open approach (allowing assignment if count retrieval fails) is appropriate to avoid blocking orders due to Redis issues. The 3+ attempts threshold and exclusion logic are correctly implemented.
1108-1142: LGTM!The attempt count check for regular orders is correctly placed after balance validation but before assignment. The implementation mirrors the OTC logic with the appropriate TTL (
OrderRequestValidity*2for regular orders). The existing recursive call toAssignPaymentOrderon failure allows the system to try another provider.tasks/tasks.go (1)
1299-1303: LGTM!The order type determination correctly maps the enum value to the string expected by
IncrementProviderAttemptCount. Using the constantpaymentorder.OrderTypeOtcensures type safety.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Description
This PR implements provider attempt tracking with retry logic to prevent immediate exclusion of providers when they fail to process orders. Instead of immediately adding providers to the exclude list on assignment failure, the system now tracks how many times each provider has been attempted for an order and only excludes them after 3 failed attempts.
Background:
Previously, when a provider failed to accept or fulfill an order (indicated by order_request expiration), they were immediately added to the exclude list. This was too aggressive and prevented providers from retrying after temporary issues like network connectivity problems or temporary service downtime.
Changes:
order_provider_attempts_{orderID}\\with provider ID as field and count as valueIncrementProviderAttemptCount\\to track attempts when order_request expiresgetProviderAttemptCount\\helper to check current attempt count for a providerassignOtcOrder\\and \sendOrderRequest\\Impact:
References
Testing
Manual Testing Steps:
Test Scenarios:
OrderRefundTimeoutOtc\\for TTLOrderRefundTimeout\\for TTLEnvironment:
Go 1.21+
Redis (for attempt tracking)
PostgreSQL (for order data)
This change adds test coverage for new/changed/fixed functionality
Checklist
main\\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.