Persist refunds and prevent cumulative over-refunds#114
Merged
Conversation
teesofttech
added
area: persistence
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughRefund processing now persists refund attempts, reserves refundable balance transactionally, handles provider outcomes, and updates payment status after confirmed refunds. EF Core indexing, dependency injection, migration guidance, and unit tests are added. ChangesRefund persistence and reservation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PaymentService
participant RefundRepository
participant PaymentGateway
participant TransactionRepository
PaymentService->>RefundRepository: Reserve refundable balance
RefundRepository-->>PaymentService: Reservation result
PaymentService->>PaymentGateway: Submit refund
PaymentGateway-->>PaymentService: Refund response or exception
PaymentService->>RefundRepository: Persist final refund status
PaymentService->>TransactionRepository: Mark transaction refunded when fully confirmed
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces persisted refund audit records and an atomic “refundable balance reservation” mechanism to prevent multiple partial refunds from cumulatively exceeding a payment’s captured amount (Issue #62). It extends the SDK’s refund flow to reserve balance before calling the provider, persist provider outcomes, and only mark a payment as fully refunded once confirmed refunds reach the captured amount.
Changes:
- Add
IRefundRepository+RefundRepositorywith a serializable reservation transaction that countsPendingandRefundedrefunds against the captured amount (excludingFailed). - Update
PaymentService.RefundPaymentAsyncto reserve balance first, persist refund attempts/results, and update the payment status only after cumulative confirmed refunds reach the captured amount. - Add EF model/index + migration (
IX_Refunds_PaymentTransactionReference_Status), documentation, and unit tests (including concurrent reservation coverage) and DI registration.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| PayBridge.SDK/Services/PaymentService.cs | Persists refund attempts/results and enforces cumulative refundable balance via repository reservation. |
| PayBridge.SDK/Repositories/RefundRepository.cs | New repository implementing reservation + persistence and lookup/update operations for refund audit rows. |
| PayBridge.SDK/Persistence/PayBridgeDbContext.cs | Adds composite index model metadata for refund reservations. |
| PayBridge.SDK/Migrations/PayBridgeDbContextModelSnapshot.cs | Updates snapshot for index + bounded PaymentTransactionReference type. |
| PayBridge.SDK/Migrations/20260718074022_AddRefundReservationIndex.Designer.cs | Migration designer for updated model. |
| PayBridge.SDK/Migrations/20260718074022_AddRefundReservationIndex.cs | Migration to alter column to nvarchar(450) and create composite index. |
| PayBridge.SDK/Interfaces/IRefundRepository.cs | Adds TryReserveAsync and makes GetByReferenceAsync nullable. |
| PayBridge.SDK/Externsions/IServiceCollectionExtensions.cs | Registers IRefundRepository in DI. |
| PayBridge.SDK.Test/Unit/RefundRepositoryTests.cs | Verifies index metadata + reservation logic + concurrency behavior. |
| PayBridge.SDK.Test/Unit/PaymentServiceRefundTests.cs | Verifies service persistence/update behavior for refunded/pending/failed paths and reservation rejection. |
| PayBridge.SDK.Test/Unit/IServiceCollectionExtensionsTests.cs | Asserts DI registration for IRefundRepository. |
| docs/refund-persistence.md | Deployment/rollback guidance + rationale for bounded reference and composite index. |
Files not reviewed (1)
- PayBridge.SDK/Migrations/20260718074022_AddRefundReservationIndex.Designer.cs: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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.Test/Unit/PaymentServiceRefundTests.cs`:
- Around line 31-37: Update the refund outcome tests around the confirmed,
pending, rejected, and exception cases to verify persistence independently of
the mutable reserved object. Assert that the repository’s UpdateAsync method is
called exactly once with the expected transaction for each outcome, including
the scenarios covered by the refund transition tests, rather than relying only
on fixture.Transaction state assertions.
In `@PayBridge.SDK.Test/Unit/RefundRepositoryTests.cs`:
- Around line 69-93: Add a provider-level concurrency test alongside
Concurrent_reservations_cannot_exceed_captured_amount that bypasses the shared
ReservationLock and executes each TryReserveAsync call through independent
database connections and contexts. Run the eight reservation attempts
concurrently, then assert exactly one succeeds and the persisted
pending/refunded total does not exceed 100m, validating cross-process
serializable behavior.
In `@PayBridge.SDK/Migrations/20260718074022_AddRefundReservationIndex.cs`:
- Around line 13-24: The migration AddRefundReservationIndex uses SQL
Server-specific nvarchar types despite being shared across PostgreSQL, MySQL,
and SQLite providers. Replace this with a provider-neutral schema change, or
split the migration into provider-specific versions while preserving the
PaymentTransactionReference alteration, index creation, and rollback behavior
for every configured provider.
In `@PayBridge.SDK/Repositories/RefundRepository.cs`:
- Around line 33-37: Update the refund validation in the repository method
containing the refund reservation logic to reject non-positive refund amounts
before calculating the reservation. Validate refund.Amount > 0 and throw the
existing out-of-range validation exception using the appropriate refund amount
parameter name, preventing negative or zero values from reaching reservedAmount
calculations.
- Around line 39-64: The TryReserveAsync refund reservation flow must execute
its user-managed transaction through the EF Core execution strategy. Wrap the
query, validation, insert, save, and commit logic in
_dbContext.Database.CreateExecutionStrategy() or ExecuteInTransactionAsync,
preserving the existing rollback and boolean outcomes. Add a provider-backed
test covering successful reservation execution with the retrying strategy
enabled.
In `@PayBridge.SDK/Services/PaymentService.cs`:
- Around line 253-264: The current refund finalization logic in PaymentService
only runs for immediate Refunded responses and is not reusable for asynchronous
confirmations. Extract it into an application-level status-update operation that
updates the refund and recalculates the related payment status transactionally,
then invoke that operation from both the immediate refund flow and
webhook/poller handling for later confirmations.
- Around line 224-251: Make the refund flow centered on the stable refund.Id
idempotency key: pass it through RefundPaymentAsync and each gateway so retries
cannot create duplicate provider refunds. When the provider succeeds but
_refundRepository.UpdateAsync fails, retain the provider result for durable
retry rather than returning an unrecoverable pending attempt. Add reconciliation
for pending refunds with uncertain provider outcomes, using the idempotency key
to query or safely retry and then persist the final result.
🪄 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: 15eccec2-cef9-4ae9-897d-52cdd3964ff7
📒 Files selected for processing (12)
PayBridge.SDK.Test/Unit/IServiceCollectionExtensionsTests.csPayBridge.SDK.Test/Unit/PaymentServiceRefundTests.csPayBridge.SDK.Test/Unit/RefundRepositoryTests.csPayBridge.SDK/Externsions/IServiceCollectionExtensions.csPayBridge.SDK/Interfaces/IRefundRepository.csPayBridge.SDK/Migrations/20260718074022_AddRefundReservationIndex.Designer.csPayBridge.SDK/Migrations/20260718074022_AddRefundReservationIndex.csPayBridge.SDK/Migrations/PayBridgeDbContextModelSnapshot.csPayBridge.SDK/Persistence/PayBridgeDbContext.csPayBridge.SDK/Repositories/RefundRepository.csPayBridge.SDK/Services/PaymentService.csdocs/refund-persistence.md
This was referenced Jul 18, 2026
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
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
IRefundRepositoryRoot cause
PaymentService.RefundPaymentAsynccompared only the current request with the original transaction amount and never persistedRefundTransaction. Multiple partial refunds could therefore independently pass validation and cumulatively exceed the captured payment.TDD
Repository and service tests were written first and observed failing before implementation. Coverage includes:
Concurrency and data model
The repository uses an in-process reservation lock plus a database
Serializabletransaction. The lock makes same-process behavior deterministic; serializable isolation and the(PaymentTransactionReference, Status)index protect the invariant across service instances.Audit rows contain normalized refund fields and
RefundResponse, not credentials, authorization headers, or raw request payloads.Migration
AddRefundReservationIndexboundsPaymentTransactionReferencetonvarchar(450)for SQL Server indexing and createsIX_Refunds_PaymentTransactionReference_Status.Before production deployment, run the documented query in
docs/refund-persistence.mdand resolve references longer than 450 characters. The down migration removes the index, restoresnvarchar(max), and retains refund rows.Validation
git diff --check: cleanCloses #62
Summary by CodeRabbit