[MINOR BC] [4.x] Fix pending tenant pull race conditions#1463
[MINOR BC] [4.x] Fix pending tenant pull race conditions#1463lukinovec wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors pullPendingFromPool() into a retrying loop that atomically claims pending tenants with a conditional update, emitting PullingPendingTenant and PendingTenantPulled events, and adds tests for concurrent stealing and transactional rollback on attribute-write failure. ChangesConcurrency-safe pending tenant pulling
sequenceDiagram
participant TestCaller
participant HasPending
participant Database
participant Tenant
TestCaller->>HasPending: pullPendingFromPool()
loop retry until success or empty
HasPending->>Database: onlyPending()->first()
alt found
HasPending->>HasPending: emit PullingPendingTenant(candidate)
HasPending->>Database: conditional update pending_since -> null (whereKey)
alt success
HasPending->>Database: findOrFail(candidate)
HasPending->>Tenant: update(attributes)
HasPending->>HasPending: emit PendingTenantPulled(tenant)
HasPending-->>TestCaller: return tenant
else lost
HasPending->>HasPending: loop again
end
else none
HasPending-->>TestCaller: create or return null
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
lukinovec
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1463 +/- ##
============================================
+ Coverage 86.00% 86.03% +0.02%
- Complexity 1175 1178 +3
============================================
Files 185 185
Lines 3429 3436 +7
============================================
+ Hits 2949 2956 +7
Misses 480 480 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
🤖 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 `@tests/PendingTenantsTest.php`:
- Around line 157-160: The test currently calls $process->wait() with no timeout
which can hang; set a reasonable timeout on each Process before waiting (e.g.,
call $process->setTimeout(10) on the $process instances in the loop or where
processes are created) and then call $process->wait(); keep the existing
assertion expect($process->isSuccessful())->toBeTrue($process->getErrorOutput())
so failures surface quickly instead of blocking CI.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6bac5abc-b34a-4fd8-8ae4-6b8c0e2e28c4
📒 Files selected for processing (3)
src/Database/Concerns/HasPending.phptests/Etc/pull-worker.phptests/PendingTenantsTest.php
stancl
left a comment
stancl
left a comment
There was a problem hiding this comment.
Race conditions aren't really something that can be tested well by brute force due to their inconsistent nature.
Instead, looking at the implementation, it seems that with the current placement of PullingPendingTenant you can easily simulate using synchronous code a case where another process "steals" a $pullCandidate without actually having to use two separate processes? Just make the listener only do this once so the following loop iteration succeeds.
That in itself should also work as a regression test I think? Should be able to write a test that'd fail with the old code but pass with the new implementation. And we wouldn't need all the concurrent tests that have been added here.
Remove the worker solution, simulate the concurrent pull by updating pending_since while PullingPendingTenant
lukinovec
changed the title
|
@coderabbitai full review |
✅ Action performedFull review finished. |
2 participants
pullPendingFromPoolhad a race condition when user A attempted to pull a tenant at the same time as user B. Both could end up grabbing the same tenant, and the result was unexpected, e.g. one of them ending up with no pending tenant pulled at all even though there was a pending tenant in the pool.instead of selecting a pending tenant and updating the same model, we now run
update()conditionally -- it clearspending_sinceonly if the tenant is still pending, and we check the affected row count. Only one process can get a row back, the other gets 0 and retries with the next pending candidate in the pool. The loop always terminates since every lost claim means the pool shrank by one. Eventually it's empty and we create a new tenant (or return null).The claim and the attribute update happen in a single transaction now, so if updating
$attributesfails, the claim rolls back and the tenant stays in the pool.Added a regression test that simulates a concurrent "steal" synchronously via a PullingPendingTenant listener. Fails with the old code, passes with the HasPending changes.
Very minor BC:
pending_sinceno longer fires model updating/updated events (since the update goes through query builder).PendingTenantPulledstill fires the same as before and is the listener you'd want to use anyway.PullingPendingTenantnow fires before the claim (and outside the transaction), so it can fire more than once with concurrent pulls (e.g. when a tenant gets claimed by someone else).PendingTenantPulledis still the one that fires exactly once for the actually pulled tenant.Summary by CodeRabbit
Bug Fixes
Tests