Add concurrent refund race condition test #1498
Open
+295
−0
Conversation
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
Contributor
Reviewer's GuideAdds a new API test module with shared fixtures and a concurrency-focused refund race condition test to verify that the select_for_update-based locking in OrderPaymentViewSet.refund() prevents double refunds under concurrent requests. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Overriding
django.test.utils.setup_databasesglobally inconftest.pywith ascopes_disabledwrapper may have unintended side effects on other test modules; consider scoping this behavior to specific tests or a dedicated test settings module instead. - The thread worker in
test_concurrent_refunds_race_condition_preventeduses the plaindjango.test.Clientrather than the configured DRFAPIClient; consider reusing the same client type or a small helper to avoid subtle differences in request handling or auth behavior. - Shared
resultsanderrorslists are mutated from multiple threads without synchronization; while list appends are atomic in CPython, using aQueueor similar thread-safe container would make the concurrency semantics clearer and more robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Overriding `django.test.utils.setup_databases` globally in `conftest.py` with a `scopes_disabled` wrapper may have unintended side effects on other test modules; consider scoping this behavior to specific tests or a dedicated test settings module instead.
- The thread worker in `test_concurrent_refunds_race_condition_prevented` uses the plain `django.test.Client` rather than the configured DRF `APIClient`; consider reusing the same client type or a small helper to avoid subtle differences in request handling or auth behavior.
- Shared `results` and `errors` lists are mutated from multiple threads without synchronization; while list appends are atomic in CPython, using a `Queue` or similar thread-safe container would make the concurrency semantics clearer and more robust.
## Individual Comments
### Comment 1
<location> `app/eventyay/api/tests/test_concurrent_refunds.py:67-76` </location>
<code_context>
+ def make_refund_request(amount):
</code_context>
<issue_to_address>
**suggestion (testing):** Add synchronization (e.g. `threading.Barrier`) so both threads start at the same time and reliably exercise the race condition.
As written, `thread1.start()` and `thread2.start()` let each thread send its HTTP request as soon as it starts, so one request can finish before the other even begins. That makes the race coverage unreliable and can cause flaky pass/fail results. Using a `threading.Barrier` (or similar) to block both threads until they’re ready, then releasing them together, will make the test more deterministic.
Suggested implementation:
```python
def make_refund_request(amount, start_barrier):
"""Make a refund request in a separate thread."""
try:
# Synchronize all worker threads so they start the HTTP request at (roughly) the same time
start_barrier.wait()
client = Client()
client.defaults['HTTP_AUTHORIZATION'] = auth_header
response = client.post(
```
To fully implement the synchronization and make the race test deterministic, you should also:
1. Import `threading` at the top of this test file:
- Add `import threading` alongside the other imports.
2. Create a barrier sized to the number of concurrent worker threads right before starting them, for example:
- `start_barrier = threading.Barrier(2)` (if there are exactly two threads in this test).
3. Pass the barrier into each thread's `args` when constructing them, updating any existing thread creation such as:
- From: `threading.Thread(target=make_refund_request, args=(Decimal("50.00"),))`
- To: `threading.Thread(target=make_refund_request, args=(Decimal("50.00"), start_barrier))`
This ensures both threads block on `start_barrier.wait()` and only proceed to send their HTTP requests once all of them are ready, making the race condition reliably exercised and reducing test flakiness.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive testing infrastructure for concurrent refund operations, specifically to validate the fix for a race condition that could allow double refunds. The test suite ensures that when two concurrent refund requests are made that would exceed the available payment amount, only one succeeds due to proper database locking mechanisms.
- Introduces shared test fixtures (organizers, events, orders, payments) for API testing
- Adds a multi-threaded concurrency test that simulates race conditions in refund processing
- Validates that select_for_update() prevents double refunds by ensuring only one of two overlapping requests succeeds
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/eventyay/api/tests/init.py | Package initialization file for API tests |
| app/eventyay/api/tests/conftest.py | Comprehensive pytest fixtures providing test data for organizers, events, teams, authenticated clients, and orders with payments |
| app/eventyay/api/tests/test_concurrent_refunds.py | Concurrency test validating race condition prevention in refund operations using threading to simulate simultaneous requests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if thread1.is_alive() or thread2.is_alive(): | ||
| pytest.fail( | ||
| f"Test threads did not finish within timeout. " | ||
| f"{{thread1 alive: {{thread1.is_alive()}}}}, thread2 alive: {{thread2.is_alive()}}}}" |
There was a problem hiding this comment.
The f-string contains double curly braces which will be printed literally instead of interpolating the variables. The braces inside an f-string should be single to enable variable interpolation.
Suggested change
| f"{{thread1 alive: {{thread1.is_alive()}}}}, thread2 alive: {{thread2.is_alive()}}}}" | |
| f"thread1 alive: {thread1.is_alive()}, thread2 alive: {thread2.is_alive()}" |
Comment on lines
+160
to
+165
| o.payments.create( | ||
| provider='manual', | ||
| state=OrderPayment.PAYMENT_STATE_CONFIRMED, | ||
| amount=Decimal('100.00'), | ||
| payment_date=datetime(2025, 1, 1, 10, 0, 0, tzinfo=timezone.utc), | ||
| ) |
There was a problem hiding this comment.
The test uses the 'manual' payment provider which does not support refunds by default. The BasePaymentProvider.payment_partial_refund_supported method returns False, and ManualPayment doesn't override it. This will cause both refund requests to fail with a 400 error at lines 1280-1283 of api/views/order.py (since the test attempts partial refunds of 80 from a 100 payment), rather than testing the intended race condition behavior. Consider using a payment provider that supports partial refunds, such as creating a test-specific payment provider that overrides payment_partial_refund_supported to return True.
Comment on lines
+52
to
+57
| client.defaults['HTTP_AUTHORIZATION'] = auth_header | ||
| response = client.post( | ||
| f'/api/v1/organizers/{organizer.slug}/events/{event.slug}/' | ||
| f'orders/{order.code}/payments/{payment.local_id}/refund/', | ||
| data=json.dumps({'amount': str(amount)}), | ||
| content_type='application/json', |
There was a problem hiding this comment.
Django's test Client does not support setting headers via client.defaults. The correct way to pass headers to Django test Client's post method is to pass them as keyword arguments directly to the post call. The line should be changed to pass HTTP_AUTHORIZATION as a parameter to the post method.
Suggested change
| client.defaults['HTTP_AUTHORIZATION'] = auth_header | |
| response = client.post( | |
| f'/api/v1/organizers/{organizer.slug}/events/{event.slug}/' | |
| f'orders/{order.code}/payments/{payment.local_id}/refund/', | |
| data=json.dumps({'amount': str(amount)}), | |
| content_type='application/json', | |
| response = client.post( | |
| f'/api/v1/organizers/{organizer.slug}/events/{event.slug}/' | |
| f'orders/{order.code}/payments/{payment.local_id}/refund/', | |
| data=json.dumps({'amount': str(amount)}), | |
| content_type='application/json', | |
| HTTP_AUTHORIZATION=auth_header, |
mariobehling
requested changes
Dec 20, 2025
Member
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Please check latest AI comments.
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.
Adding tests for as mentioned in the pr #1406
Summary by Sourcery
Add API test fixtures and a concurrency test to verify refund race condition handling.
Tests: