[ENG-401] fix: token queue should respect sub queues while calling serve next#3708
Conversation
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesToken update validation now handles sub-queue conflicts and status transitions separately. Token queue consistency
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
nandkishorr
changed the title
Greptile SummaryThis PR tightens sub-queue token management: direct
Confidence Score: 3/5The lock key is now correctly scoped to the sub_queue, but the sub_queue object is loaded before the lock is acquired, so the current_token guard inside the critical section can act on stale data and allow two tokens to enter IN_PROGRESS on the same sub-queue. The lock key change is correct in principle, but any caller that loads sub_queue before a prior call commits and then acquires the released lock will bypass the current_token guard using a stale snapshot, silently producing duplicate IN_PROGRESS tokens. care/emr/api/viewsets/scheduling/token.py — the set_next action needs sub_queue re-fetched with select_for_update inside the lock+transaction block. Important Files Changed
Reviews (6): Last reviewed commit: "added lock" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3708 +/- ##
===========================================
- Coverage 79.67% 79.65% -0.03%
===========================================
Files 479 479
Lines 23098 23107 +9
Branches 2406 2409 +3
===========================================
+ Hits 18404 18406 +2
- Misses 4086 4090 +4
- Partials 608 611 +3 ☔ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
care/emr/tests/test_token_api.py (1)
278-292: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMissing test coverage for new sub-queue validation and clearing behavior.
The status change from
IN_PROGRESStoCREATEDis correct — the newperform_updatelogic (lines 113-120 intoken.py) would block assigning a sub_queue while transitioning toIN_PROGRESS. However, the new behavior itself is entirely untested:
- Sub_queue change during IN_PROGRESS is blocked — no test verifies that updating a token to
IN_PROGRESSwhile changing its sub_queue returns 400 with "Use set_next endpoint to change the sub queue of a token".- Current_token conflict on IN_PROGRESS with same sub_queue — no test verifies that setting
IN_PROGRESSwith an existing sub_queue that has a differentcurrent_tokenreturns 400 with "Sub Queue already has a current token".- Current_token clearing on status transition away from IN_PROGRESS — no test verifies that updating a token from
IN_PROGRESStoFULFILLED/CANCELLEDclears the sub_queue'scurrent_token.- Next-token prioritization (
token_queue.pylines 235-238) — no test verifies thatset_next_token_to_subqueueprefers tokens already assigned to the requested sub_queue over unassigned waiting tokens.The existing tests (
test_update_token_with_existing_current_token_in_subqueue,test_update_token_with_current_token_as_null_in_subqueue) cover the firstvalidate_datablock but none exercise theIN_PROGRESS-gated paths.Would you like me to generate the missing test cases?
Also applies to: 314-328
🤖 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 `@care/emr/tests/test_token_api.py` around lines 278 - 292, The token API tests lack coverage for the new sub-queue validation and current-token lifecycle behavior. Add focused tests around the existing update-token test cases and `perform_update` covering: rejecting an IN_PROGRESS update that changes sub_queue with the specified 400 message; rejecting IN_PROGRESS when the sub_queue has a different current_token; clearing current_token when transitioning from IN_PROGRESS to FULFILLED or CANCELLED; and verifying `set_next_token_to_subqueue` prioritizes tokens already assigned to the requested sub_queue over unassigned waiting tokens.
🤖 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.
Outside diff comments:
In `@care/emr/tests/test_token_api.py`:
- Around line 278-292: The token API tests lack coverage for the new sub-queue
validation and current-token lifecycle behavior. Add focused tests around the
existing update-token test cases and `perform_update` covering: rejecting an
IN_PROGRESS update that changes sub_queue with the specified 400 message;
rejecting IN_PROGRESS when the sub_queue has a different current_token; clearing
current_token when transitioning from IN_PROGRESS to FULFILLED or CANCELLED; and
verifying `set_next_token_to_subqueue` prioritizes tokens already assigned to
the requested sub_queue over unassigned waiting tokens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4955bd10-01bf-440e-9009-38445ca4c38a
📒 Files selected for processing (3)
care/emr/api/viewsets/scheduling/token.pycare/emr/api/viewsets/scheduling/token_queue.pycare/emr/tests/test_token_api.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
care/emr/api/viewsets/scheduling/token.py (2)
107-115: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd the resource guard here too. This branch only catches sub-queue swaps, so a token with no existing sub-queue can still attach one from the wrong resource. Match the create path and reject
instance.sub_queue.resource != obj.queue.resourcebefore the current transition check.🤖 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 `@care/emr/api/viewsets/scheduling/token.py` around lines 107 - 115, Update the validation logic containing the existing sub-queue transition check to also guard the target queue’s resource: reject when obj.sub_queue exists and obj.sub_queue.resource differs from obj.queue.resource, before evaluating the current instance transition condition. Keep the existing mismatch check for sub-queue swaps so both creation-like attachments and transitions enforce resource consistency.
201-213: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLock the sub-queue row in
set_next.transaction.atomic()doesn’t serialize concurrent calls here, so two requests can both pass thecurrent_tokencheck and one will quietly overwrite the other. Useselect_for_update()onTokenSubQueuelike the create path does.🤖 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 `@care/emr/api/viewsets/scheduling/token.py` around lines 201 - 213, Update set_next to retrieve the TokenSubQueue through select_for_update() before checking or assigning current_token, matching the locking approach used by the create path; keep the lookup inside the existing transaction.atomic() block so concurrent calls serialize safely.
🤖 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.
Outside diff comments:
In `@care/emr/api/viewsets/scheduling/token.py`:
- Around line 107-115: Update the validation logic containing the existing
sub-queue transition check to also guard the target queue’s resource: reject
when obj.sub_queue exists and obj.sub_queue.resource differs from
obj.queue.resource, before evaluating the current instance transition condition.
Keep the existing mismatch check for sub-queue swaps so both creation-like
attachments and transitions enforce resource consistency.
- Around line 201-213: Update set_next to retrieve the TokenSubQueue through
select_for_update() before checking or assigning current_token, matching the
locking approach used by the create path; keep the lookup inside the existing
transaction.atomic() block so concurrent calls serialize safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cedfac83-4a0d-49cd-8098-4162ab0c84d8
📒 Files selected for processing (1)
care/emr/api/viewsets/scheduling/token.py
nandkishorr
force-pushed
the
ENG-401-token-queue-should-respect-sub-queues-while-calling-serve-next
branch
from
d82319b to
5ada8a3
Compare
| def validate_data(self, instance, model_obj=None): | ||
| if ( | ||
| model_obj | ||
| and instance.sub_queue | ||
| and model_obj.sub_queue | ||
| and instance.sub_queue != model_obj.sub_queue.external_id | ||
| ): | ||
| existing_current = TokenSubQueue.objects.filter( | ||
| current_token=model_obj | ||
| ).exists() | ||
| if existing_current: | ||
| raise ValidationError("Sub Queue already has a current token") | ||
| if model_obj and instance.sub_queue: | ||
| if ( | ||
| model_obj.sub_queue | ||
| and instance.sub_queue != model_obj.sub_queue.external_id | ||
| ): | ||
| existing_current = TokenSubQueue.objects.filter( | ||
| current_token=model_obj | ||
| ).exists() | ||
| if existing_current: | ||
| raise ValidationError("Sub Queue already has a current token") | ||
| if ( | ||
| instance.sub_queue | ||
| and instance.status == TokenStatusOptions.IN_PROGRESS.value | ||
| ): | ||
| raise ValidationError("Use set_next endpoint to assign a token.") | ||
|
|
||
| return super().validate_data(instance, model_obj) |
There was a problem hiding this comment.
sub_queue is null
The new validation at lines 94–98 that blocks direct IN_PROGRESS updates lives inside if model_obj and instance.sub_queue:. A caller that sends {"status": "in_progress", "sub_queue": null} satisfies pydantic (since TokenUpdateSpec.sub_queue: UUID4 | None accepts null) but makes instance.sub_queue falsy, so the entire guard is skipped. The token then transitions to IN_PROGRESS with sub_queue = null in perform_update — the old sub-queue's current_token gets cleared, but nothing sets a new current_token, leaving the queue in an inconsistent state.
The IN_PROGRESS status check should be guarded on model_obj alone, independent of whether a sub-queue is being provided in the same request.
nandkishorr
force-pushed
the
ENG-401-token-queue-should-respect-sub-queues-while-calling-serve-next
branch
from
5ada8a3 to
b21fa4a
Compare
| sub_queue = get_object_or_404( | ||
| TokenSubQueue, | ||
| external_id=request_obj.sub_queue, | ||
| resource=queue.resource, | ||
| ) | ||
| with Lock(f"token:set_next:{sub_queue.id}"), transaction.atomic(): | ||
| if sub_queue.current_token and sub_queue.current_token != obj: | ||
| raise ValidationError("Sub Queue already has a current token") | ||
| sub_queue.current_token = obj | ||
| sub_queue.save() | ||
| obj.status = TokenStatusOptions.IN_PROGRESS.value | ||
| obj.sub_queue = sub_queue | ||
| obj.save() |
There was a problem hiding this comment.
sub_queue snapshot inside the lock
sub_queue is fetched via get_object_or_404 (lines 201–205) before the lock is acquired. The lock serialises concurrent calls, but it does not refresh the in-memory object. A second request that fetches sub_queue (seeing current_token = None) and then acquires the lock after the first request has already committed will still evaluate sub_queue.current_token against the old cached value — both pass the guard and both tokens become IN_PROGRESS on the same sub-queue simultaneously.
Re-fetch sub_queue with select_for_update() inside the lock+transaction so the guard always reflects the current DB state:
with Lock(f"token:set_next:{sub_queue.id}"), transaction.atomic():
sub_queue = TokenSubQueue.objects.select_for_update().get(id=sub_queue.id)
if sub_queue.current_token and sub_queue.current_token != obj:
raise ValidationError("Sub Queue already has a current token")
...1 participant
Proposed Changes
Associated Issue
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
set-nextrules: only transitions fromCREATEDare allowed, prevents overwriting a sub-queue with a different current token, and improves next-token selection priority.CREATEDand assertCREATEDresults.