Implement refund idempotency and transactional reconciliation (#115 #116 #117)#128
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefund processing now supports idempotency keys and request fingerprints, replays matching stored outcomes, rejects mismatched retries, and finalizes refund and payment state transactionally through the repository. ChangesRefund idempotency and finalization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PaymentService
participant RefundRepository
participant PaymentGateway
Client->>PaymentService: Submit refund with IdempotencyKey
PaymentService->>RefundRepository: Look up stored refund
RefundRepository-->>PaymentService: Stored refund or no match
PaymentService->>PaymentGateway: Execute refund for new request
PaymentGateway-->>PaymentService: RefundResponse
PaymentService->>RefundRepository: Finalize refund and payment state
RefundRepository-->>PaymentService: Finalized refund
PaymentService-->>Client: RefundResponse
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
Pull request overview
This PR adds application-level refund idempotency and introduces a transactional “finalize refund” operation in the persistence layer so refund completion and parent payment status reconciliation can be persisted atomically.
Changes:
- Add refund idempotency via an
IdempotencyKey+ stable request fingerprint and replay logic inPaymentService. - Add
FinalizeAsyncinRefundRepositoryto persist refund status + reconcile payment status within a serializable transaction. - Add EF Core schema changes (columns + index) and expand refund unit tests for idempotent replay and mismatched-payload rejection.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| PayBridge.SDK/Services/PaymentService.cs | Adds refund idempotency replay/mismatch checks and routes refund persistence through transactional finalization. |
| PayBridge.SDK/Repositories/RefundRepository.cs | Adds GetByIdempotencyKeyAsync and FinalizeAsync to reconcile refund + payment state transactionally. |
| PayBridge.SDK/Persistence/PayBridgeDbContext.cs | Adds refund idempotency index and column constraints in the EF model. |
| PayBridge.SDK/Migrations/PayBridgeDbContextModelSnapshot.cs | Updates model snapshot to include refund idempotency key + request fingerprint. |
| PayBridge.SDK/Migrations/20260719140635_AddRefundIdempotency.Designer.cs | New migration designer snapshot for the refund idempotency schema changes. |
| PayBridge.SDK/Migrations/20260719140635_AddRefundIdempotency.cs | New migration adding refund idempotency columns and unique index. |
| PayBridge.SDK/Interfaces/IRefundRepository.cs | Extends repository contract with idempotency lookup and transactional finalization API. |
| PayBridge.SDK/Entities/RefundTransaction.cs | Adds IdempotencyKey and RequestFingerprint fields to persisted refund entity. |
| PayBridge.SDK/Dtos/Request/RefundRequest.cs | Adds optional IdempotencyKey to refund request DTO. |
| PayBridge.SDK.Test/Unit/PaymentServiceRefundTests.cs | Adds tests for idempotent replay and mismatched-payload idempotency rejection; updates expectations for finalization. |
Files not reviewed (1)
- PayBridge.SDK/Migrations/20260719140635_AddRefundIdempotency.Designer.cs: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!string.IsNullOrWhiteSpace(request.IdempotencyKey)) | ||
| { | ||
| var existingRefund = await _refundRepository.GetByIdempotencyKeyAsync(request.IdempotencyKey); |
| Amount = request.Amount, | ||
| Currency = transaction.Currency, | ||
| Reason = request.Reason, |
| if (string.IsNullOrWhiteSpace(refund.RequestFingerprint)) | ||
| { | ||
| refund.RequestFingerprint = string.Empty; | ||
| } |
| modelBuilder.Entity<RefundTransaction>() | ||
| .HasIndex(refund => refund.IdempotencyKey) | ||
| .IsUnique() | ||
| .HasFilter("[IdempotencyKey] IS NOT NULL"); |
| [Fact] | ||
| public async Task RefundPaymentAsync_replays_stored_response_for_same_idempotency_request() | ||
| { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
PayBridge.SDK/Services/PaymentService.cs (2)
412-424: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
IdempotencyKeydefaults to empty string instead ofnull, breaking the filtered unique index for every non-idempotent refund.The filtered unique index on
RefundTransaction.IdempotencyKeyisWHERE [IdempotencyKey] IS NOT NULL(seePayBridgeDbContext.cs), meaning it should only constrain refunds that actually supply a key. Coalescing tostring.Emptyhere makes every key-less refund store"", which is not null, so the second refund without an idempotency key anywhere in the system will violate the unique index and throw an unhandledDbUpdateExceptionfromTryReserveAsync. Compare with the payment path, where non-idempotentPaymentTransactions simply leaveIdempotencyKeyunset (null).🐛 Proposed fix
- IdempotencyKey = request.IdempotencyKey ?? string.Empty, + IdempotencyKey = request.IdempotencyKey,🤖 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 `@PayBridge.SDK/Services/PaymentService.cs` around lines 412 - 424, Update the RefundTransaction initializer in the refund creation flow to assign request.IdempotencyKey directly, preserving null for requests without an idempotency key. Keep the existing value unchanged when a key is supplied and align this behavior with the PaymentTransaction creation path.
374-433: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRefund idempotency path lacks the concurrency safeguards used in the analogous payment path.
CreateIdempotentPaymentAsyncprotects against racing duplicate requests with an in-process semaphore lock (AcquireIdempotencyLock) plus acatch (DbUpdateException)fallback that re-fetches and replays the winning attempt. The refund path here has neither: two concurrent requests with the sameIdempotencyKeycan both pass theexistingRefund is nullcheck and both callTryReserveAsync, so the loser's insert will throw an unhandledDbUpdateExceptionfrom the new unique index instead of gracefully replaying the stored outcome. Separately, there's no length check onrequest.IdempotencyKeybefore it's persisted into thenvarchar(255)column (the payment path validatesLength > 255up front).Recommend mirroring the payment path: acquire a per-key lock around the lookup/create/reserve sequence, catch
DbUpdateExceptionaroundTryReserveAsyncto re-fetch and replay via the existing lookup logic, and validate key length early.🤖 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 `@PayBridge.SDK/Services/PaymentService.cs` around lines 374 - 433, The refund idempotency flow in the enclosing refund method lacks the safeguards used by CreateIdempotentPaymentAsync. Validate request.IdempotencyKey length before persistence, acquire the existing per-key lock via AcquireIdempotencyLock around the lookup/create/TryReserveAsync sequence, and handle DbUpdateException from TryReserveAsync by re-fetching the existing refund and replaying it through the current stored-response/result logic.
🧹 Nitpick comments (1)
PayBridge.SDK.Test/Unit/PaymentServiceRefundTests.cs (1)
203-225: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftMocked
FinalizeAsync/TryReserveAsynccan't catch real EF-layer bugs (e.g., unique-index or query-ordering defects inRefundRepository).This fixture hand-rolls the finalize/reserve behavior instead of exercising a real
DbContext, so it can't surface issues like a filtered unique index being violated by an unintended non-null value, or a SUM query racing ahead of a pendingSaveChangesAsync(). Given the PR objective calls for "provider-backed contention tests using independent contexts," consider adding an EF Core-backed (Sqlite/InMemory or real provider) integration test suite specifically forRefundRepositoryto close this gap.🤖 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 `@PayBridge.SDK.Test/Unit/PaymentServiceRefundTests.cs` around lines 203 - 225, Replace the hand-rolled FinalizeAsync/TryReserveAsync mock coverage in the refund tests with an EF Core-backed integration suite for RefundRepository using independent DbContext instances. Exercise provider-backed contention and persistence flows so unique-index violations, query ordering, and pending SaveChangesAsync behavior are validated against the real repository implementation rather than mocked callbacks.
🤖 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 `@PayBridge.SDK/Repositories/RefundRepository.cs`:
- Around line 145-174: Persist the updated trackedRefund before calculating
confirmedAmount in the refund finalization flow, ensuring the database aggregate
includes the current refund’s new status and amount. Reorder the
_dbContext.Refunds.Update(trackedRefund) and SaveChangesAsync calls ahead of the
PaymentStatus.Refunded check, while preserving the existing payment update,
transaction commit, and return behavior.
In `@PayBridge.SDK/Services/PaymentService.cs`:
- Around line 440-454: Update the exception handling around the refund gateway
call in the refund-processing method so transient or otherwise uncertain
provider failures are not finalized as PaymentStatus.Failed. Persist an explicit
uncertain/pending outcome, then use the provider’s refund-status query and
existing idempotency flow to reconcile whether the refund was accepted before
returning or throwing; ensure retries do not replay a definitive failure while
reconciliation remains unresolved.
---
Outside diff comments:
In `@PayBridge.SDK/Services/PaymentService.cs`:
- Around line 412-424: Update the RefundTransaction initializer in the refund
creation flow to assign request.IdempotencyKey directly, preserving null for
requests without an idempotency key. Keep the existing value unchanged when a
key is supplied and align this behavior with the PaymentTransaction creation
path.
- Around line 374-433: The refund idempotency flow in the enclosing refund
method lacks the safeguards used by CreateIdempotentPaymentAsync. Validate
request.IdempotencyKey length before persistence, acquire the existing per-key
lock via AcquireIdempotencyLock around the lookup/create/TryReserveAsync
sequence, and handle DbUpdateException from TryReserveAsync by re-fetching the
existing refund and replaying it through the current stored-response/result
logic.
---
Nitpick comments:
In `@PayBridge.SDK.Test/Unit/PaymentServiceRefundTests.cs`:
- Around line 203-225: Replace the hand-rolled FinalizeAsync/TryReserveAsync
mock coverage in the refund tests with an EF Core-backed integration suite for
RefundRepository using independent DbContext instances. Exercise provider-backed
contention and persistence flows so unique-index violations, query ordering, and
pending SaveChangesAsync behavior are validated against the real repository
implementation rather than mocked callbacks.
🪄 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: d482c5f4-2ee4-4254-8668-f98a847db69d
📒 Files selected for processing (10)
PayBridge.SDK.Test/Unit/PaymentServiceRefundTests.csPayBridge.SDK/Dtos/Request/RefundRequest.csPayBridge.SDK/Entities/RefundTransaction.csPayBridge.SDK/Interfaces/IRefundRepository.csPayBridge.SDK/Migrations/20260719140635_AddRefundIdempotency.Designer.csPayBridge.SDK/Migrations/20260719140635_AddRefundIdempotency.csPayBridge.SDK/Migrations/PayBridgeDbContextModelSnapshot.csPayBridge.SDK/Persistence/PayBridgeDbContext.csPayBridge.SDK/Repositories/RefundRepository.csPayBridge.SDK/Services/PaymentService.cs
| trackedRefund.RefundReference = string.IsNullOrWhiteSpace(response.RefundReference) | ||
| ? trackedRefund.Id | ||
| : response.RefundReference; | ||
| trackedRefund.Status = response.Success ? response.Status : PaymentStatus.Failed; | ||
| trackedRefund.ProcessedAt = trackedRefund.Status == PaymentStatus.Pending | ||
| ? null | ||
| : response.RefundDate == default ? DateTime.UtcNow : response.RefundDate; | ||
| trackedRefund.GatewayResponse = JsonSerializer.Serialize(response); | ||
|
|
||
| if (trackedRefund.Status == PaymentStatus.Refunded) | ||
| { | ||
| var confirmedAmount = await _dbContext.Refunds | ||
| .Where(item => | ||
| item.PaymentTransactionReference == trackedRefund.PaymentTransactionReference && | ||
| item.Status == PaymentStatus.Refunded) | ||
| .Select(item => item.Amount) | ||
| .SumAsync(); | ||
|
|
||
| if (confirmedAmount >= payment.Amount) | ||
| { | ||
| payment.Status = PaymentStatus.Refunded; | ||
| _dbContext.Transactions.Update(payment); | ||
| } | ||
| } | ||
|
|
||
| _dbContext.Refunds.Update(trackedRefund); | ||
| await _dbContext.SaveChangesAsync(); | ||
| await transaction.CommitAsync(); | ||
| return trackedRefund; | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Payment reconciliation undercounts the refund currently being finalized.
confirmedAmount is computed via a SQL aggregate query (Line 156-161) before trackedRefund's new Status is persisted via SaveChangesAsync() (Line 171). Since the sum is translated to SQL and executed against the database, it reflects the pre-update row for trackedRefund, so the refund being finalized right now is never included in its own confirmed-amount check. A single full refund will therefore never flip payment.Status to Refunded here, and partial refunds are permanently under-counted by the current refund's amount. The provided mocked unit tests can't catch this because they don't execute real SQL.
🐛 Proposed fix: persist the refund transition before computing the confirmed total
- trackedRefund.GatewayResponse = JsonSerializer.Serialize(response);
-
- if (trackedRefund.Status == PaymentStatus.Refunded)
- {
- var confirmedAmount = await _dbContext.Refunds
- .Where(item =>
- item.PaymentTransactionReference == trackedRefund.PaymentTransactionReference &&
- item.Status == PaymentStatus.Refunded)
- .Select(item => item.Amount)
- .SumAsync();
-
- if (confirmedAmount >= payment.Amount)
- {
- payment.Status = PaymentStatus.Refunded;
- _dbContext.Transactions.Update(payment);
- }
- }
-
- _dbContext.Refunds.Update(trackedRefund);
- await _dbContext.SaveChangesAsync();
+ trackedRefund.GatewayResponse = JsonSerializer.Serialize(response);
+
+ _dbContext.Refunds.Update(trackedRefund);
+ await _dbContext.SaveChangesAsync();
+
+ if (trackedRefund.Status == PaymentStatus.Refunded)
+ {
+ var confirmedAmount = await _dbContext.Refunds
+ .Where(item =>
+ item.PaymentTransactionReference == trackedRefund.PaymentTransactionReference &&
+ item.Status == PaymentStatus.Refunded)
+ .Select(item => item.Amount)
+ .SumAsync();
+
+ if (confirmedAmount >= payment.Amount)
+ {
+ payment.Status = PaymentStatus.Refunded;
+ _dbContext.Transactions.Update(payment);
+ await _dbContext.SaveChangesAsync();
+ }
+ }
await transaction.CommitAsync();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| trackedRefund.RefundReference = string.IsNullOrWhiteSpace(response.RefundReference) | |
| ? trackedRefund.Id | |
| : response.RefundReference; | |
| trackedRefund.Status = response.Success ? response.Status : PaymentStatus.Failed; | |
| trackedRefund.ProcessedAt = trackedRefund.Status == PaymentStatus.Pending | |
| ? null | |
| : response.RefundDate == default ? DateTime.UtcNow : response.RefundDate; | |
| trackedRefund.GatewayResponse = JsonSerializer.Serialize(response); | |
| if (trackedRefund.Status == PaymentStatus.Refunded) | |
| { | |
| var confirmedAmount = await _dbContext.Refunds | |
| .Where(item => | |
| item.PaymentTransactionReference == trackedRefund.PaymentTransactionReference && | |
| item.Status == PaymentStatus.Refunded) | |
| .Select(item => item.Amount) | |
| .SumAsync(); | |
| if (confirmedAmount >= payment.Amount) | |
| { | |
| payment.Status = PaymentStatus.Refunded; | |
| _dbContext.Transactions.Update(payment); | |
| } | |
| } | |
| _dbContext.Refunds.Update(trackedRefund); | |
| await _dbContext.SaveChangesAsync(); | |
| await transaction.CommitAsync(); | |
| return trackedRefund; | |
| }); | |
| trackedRefund.RefundReference = string.IsNullOrWhiteSpace(response.RefundReference) | |
| ? trackedRefund.Id | |
| : response.RefundReference; | |
| trackedRefund.Status = response.Success ? response.Status : PaymentStatus.Failed; | |
| trackedRefund.ProcessedAt = trackedRefund.Status == PaymentStatus.Pending | |
| ? null | |
| : response.RefundDate == default ? DateTime.UtcNow : response.RefundDate; | |
| trackedRefund.GatewayResponse = JsonSerializer.Serialize(response); | |
| _dbContext.Refunds.Update(trackedRefund); | |
| await _dbContext.SaveChangesAsync(); | |
| if (trackedRefund.Status == PaymentStatus.Refunded) | |
| { | |
| var confirmedAmount = await _dbContext.Refunds | |
| .Where(item => | |
| item.PaymentTransactionReference == trackedRefund.PaymentTransactionReference && | |
| item.Status == PaymentStatus.Refunded) | |
| .Select(item => item.Amount) | |
| .SumAsync(); | |
| if (confirmedAmount >= payment.Amount) | |
| { | |
| payment.Status = PaymentStatus.Refunded; | |
| _dbContext.Transactions.Update(payment); | |
| await _dbContext.SaveChangesAsync(); | |
| } | |
| } | |
| await transaction.CommitAsync(); | |
| return trackedRefund; | |
| }); |
🤖 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 `@PayBridge.SDK/Repositories/RefundRepository.cs` around lines 145 - 174,
Persist the updated trackedRefund before calculating confirmedAmount in the
refund finalization flow, ensuring the database aggregate includes the current
refund’s new status and amount. Reorder the
_dbContext.Refunds.Update(trackedRefund) and SaveChangesAsync calls ahead of the
PaymentStatus.Refunded check, while preserving the existing payment update,
transaction commit, and return behavior.
| catch (Exception ex) | ||
| { | ||
| refund.Status = PaymentStatus.Failed; | ||
| refund.ProcessedAt = DateTime.UtcNow; | ||
| refund.GatewayResponse = JsonSerializer.Serialize(new | ||
| await _refundRepository.FinalizeAsync(refund, new RefundResponse | ||
| { | ||
| ErrorType = ex.GetType().Name | ||
| Success = false, | ||
| TransactionReference = request.TransactionReference, | ||
| RefundReference = refund.RefundReference, | ||
| Amount = request.Amount, | ||
| Status = PaymentStatus.Failed, | ||
| Message = ex.Message, | ||
| RefundDate = DateTime.UtcNow | ||
| }); | ||
| await _refundRepository.UpdateAsync(refund); | ||
| _logger.LogError(ex, "Refund processing failed with {Gateway}", selectedGateway); | ||
| throw new PaymentGatewayException($"Refund processing failed with {selectedGateway}", ex); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Gateway exceptions are durably finalized as Failed with no reconciliation path for uncertain provider outcomes.
Any exception from the gateway call — including transient/network errors where the refund may have actually been accepted upstream — is unconditionally finalized as PaymentStatus.Failed, and that outcome is durably serialized into GatewayResponse. A subsequent retry with the same idempotency key will simply replay this stored "Failed" response (lines 389-395) rather than re-querying the provider, so there is no way to detect or reconcile a refund that actually succeeded despite the client-side exception.
Based on learnings, this repeats the exact gap flagged previously on this file: refund safety should come from idempotency and the ability to query refund status from the provider, with durable persistence of uncertain outcomes and reconciliation logic — not a partial implementation.
🤖 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 `@PayBridge.SDK/Services/PaymentService.cs` around lines 440 - 454, Update the
exception handling around the refund gateway call in the refund-processing
method so transient or otherwise uncertain provider failures are not finalized
as PaymentStatus.Failed. Persist an explicit uncertain/pending outcome, then use
the provider’s refund-status query and existing idempotency flow to reconcile
whether the refund was accepted before returning or throwing; ensure retries do
not replay a definitive failure while reconciliation remains unresolved.
Source: Learnings
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Summary
Validation
PaymentServiceRefundTests|FullyQualifiedNameRefundRepositoryTests"Closes #115
Closes #116
Closes #117
Summary by CodeRabbit