Refuse queue deletion when routing rules still reference it - #808
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughQueue deletion now rejects queues referenced by routing rules, returns structured dependent-rule errors, and displays those errors in the dashboard. Database constraints, server handling, service wiring, regression tests, and client modal rendering are updated. ChangesQueue deletion FK enforcement and error feedback
Investigation type cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ManualReviewQueuesDashboard
participant GraphQLMutation
participant QueueOperations
participant PostgreSQL
ManualReviewQueuesDashboard->>GraphQLMutation: request queue deletion
GraphQLMutation->>QueueOperations: deleteManualReviewQueue
QueueOperations->>PostgreSQL: delete queue row
PostgreSQL-->>QueueOperations: FK violation
QueueOperations->>PostgreSQL: query dependent rule names
PostgreSQL-->>QueueOperations: rule names
QueueOperations-->>GraphQLMutation: structured 409 error
GraphQLMutation-->>ManualReviewQueuesDashboard: error message and rule names
ManualReviewQueuesDashboard->>ManualReviewQueuesDashboard: render deletion error modal
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![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 (1)
server/services/manualReviewToolService/modules/QueueOperations.ts (1)
393-445:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDeletion failure now still destroys Redis jobs before returning 409.
With the new FK-reject path (Line 419 onward), the method can fail after
queue.obliterate({ force: true })has already run (Line 391), so users get “could not delete queue” but pending jobs are already gone. Please move obliteration after a successful DB delete path (numDeletedRows === 1n).Suggested fix
- const queue = await this.getOrCreateBullQueue({ orgId, queueId }); - - await queue.obliterate({ force: true }); + const queue = await this.getOrCreateBullQueue({ orgId, queueId }); let numDeletedRows: bigint; try { numDeletedRows = await this.transactionWithRetry(async (transaction) => { @@ throw e; } - return numDeletedRows === 1n; + if (numDeletedRows !== 1n) { + return false; + } + + await queue.obliterate({ force: true }); + return true; }🤖 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 `@server/services/manualReviewToolService/modules/QueueOperations.ts` around lines 393 - 445, The queue.obliterate method call is currently executing before the database transaction that deletes the queue, causing Redis jobs to be destroyed even when the database deletion fails due to foreign key constraints. Move the queue.obliterate call to execute only after the transactionWithRetry completes successfully and numDeletedRows is greater than 0, ensuring that Redis jobs are only deleted if the database deletion succeeds and does not trigger the isForeignKeyViolationError path that throws an error response.
🤖 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 `@client/src/webpages/dashboard/mrt/ManualReviewQueuesDashboard.tsx`:
- Around line 227-230: The code in the JSON.parse block for ruleNames does not
validate that the parsed result is actually an array before assignment, which
can cause a runtime error when the render code at line 487 attempts to call .map
on ruleNames. Add a runtime validation check after JSON.parse(rawDetail) to
ensure the parsed value is an array before assigning it to ruleNames. If the
validation fails, keep ruleNames as an empty array or undefined to prevent
errors in the downstream .map call.
In `@server/services/manualReviewToolService/modules/QueueOperations.ts`:
- Around line 419-445: The current implementation assumes all foreign key
violation errors are due to routing rule dependencies, but this could be
incorrect if new RESTRICT foreign keys are added to the manual_review_queues
table in the future. Add a constraint name check within the
isForeignKeyViolationError(e) block to verify that e.constraint matches one of
the two expected routing rule constraint names
(routing_rules_destination_queue_id_fkey or
appeals_routing_rules_destination_queue_id_fkey) before executing the query
logic and throwing makeQueueHasDependentRoutingRulesError. If the constraint
name does not match either of these, skip the query block and simply throw the
original error instead.
---
Outside diff comments:
In `@server/services/manualReviewToolService/modules/QueueOperations.ts`:
- Around line 393-445: The queue.obliterate method call is currently executing
before the database transaction that deletes the queue, causing Redis jobs to be
destroyed even when the database deletion fails due to foreign key constraints.
Move the queue.obliterate call to execute only after the transactionWithRetry
completes successfully and numDeletedRows is greater than 0, ensuring that Redis
jobs are only deleted if the database deletion succeeds and does not trigger the
isForeignKeyViolationError path that throws an error response.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 43f18fd4-9cb8-4675-8342-cbfcc0df4d9e
📒 Files selected for processing (5)
client/src/webpages/dashboard/mrt/ManualReviewQueuesDashboard.tsxdb/src/scripts/api-server-pg/2026.06.17T18.15.16.restrict_routing_rules_queue_fkeys.sqlserver/services/manualReviewToolService/modules/JobRouting.test.tsserver/services/manualReviewToolService/modules/QueueOperations.test.tsserver/services/manualReviewToolService/modules/QueueOperations.ts
There was a problem hiding this comment.
Pull request overview
Prevents manual review queue deletion from silently breaking routing by changing DB foreign keys from ON DELETE CASCADE to ON DELETE RESTRICT, then surfacing a specific conflict error to users listing the routing rules that block deletion.
Changes:
- Add a Postgres migration to restrict deletion of queues referenced by
routing_rules/appeals_routing_rules. - Server: catch FK violations during queue deletion, look up blocking rule names, and throw
QueueHasDependentRoutingRulesError(409) with rule names indetail. - Client + tests: show a modal listing blocking rule names and add regression tests covering both routing-rule tables.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/services/manualReviewToolService/modules/QueueOperations.ts | Catch FK violations on queue delete and raise a 409 error containing blocking routing rule names; update test-only delete helper for RESTRICT behavior. |
| server/services/manualReviewToolService/modules/QueueOperations.test.ts | Add regression tests ensuring queue deletion rejects when routing rules reference the queue. |
| server/services/manualReviewToolService/modules/JobRouting.test.ts | Update test commentary to reflect RESTRICT FK behavior and the test-only deletion helper’s semantics. |
| db/src/scripts/api-server-pg/2026.06.17T18.15.16.restrict_routing_rules_queue_fkeys.sql | Change the two destination-queue FKs from CASCADE to RESTRICT. |
| client/src/webpages/dashboard/mrt/ManualReviewQueuesDashboard.tsx | Surface server deletion errors via a modal and link users to the routing rules dashboard. |
Fixes issue roostorg#738 (design #1). Changes the FK constraints on routing_rules.destination_queue_id and appeals_routing_rules.destination_queue_id from ON DELETE CASCADE to ON DELETE RESTRICT. The service now catches the resulting FK violation and throws QueueHasDependentRoutingRulesError, naming the blocking rules, instead of silently cascade-deleting them and breaking routing. The client's previously silent onError handler is replaced with a modal that surfaces the server's error message so the user knows which rules to update before retrying. Regression tests cover both routing_rules and appeals_routing_rules. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… and link to rules page
deleteManualReviewQueueForTestsDO_NOT_USE was written when routing_rules had CASCADE FKs on destination_queue_id. Now that the FK is RESTRICT, the helper must explicitly delete referencing routing rules and appeals routing rules before removing the queue, or the DB rejects the delete. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move queue.obliterate() after DB transaction so Redis jobs aren't destroyed if deletion is blocked by FK constraints - Gate FK error mapping on specific constraint names to avoid swallowing unrelated 23503 errors from future RESTRICT FKs - Add org_id filter to routing rule deletes in test helper - Validate JSON.parse result is string[] before assigning to ruleNames - Use index as React key to avoid collisions on duplicate rule names Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
26b6c85 to
32653b3
Compare
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 `@server/services/manualReviewToolService/modules/QueueOperations.ts`:
- Around line 508-512: Update the deleteManualReviewQueue flow around
queue.obliterate({ force: true }) to catch and explicitly log or otherwise
surface obliteration failures after the database delete commits, referencing the
existing recovery path recover-mrt-queue.ts. Preserve the committed deletion
result while ensuring retries cannot silently treat the missing row as a
successful no-op and hide the orphaned Redis queue state.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 56247178-6a9b-4db5-b410-fdd9d360046f
📒 Files selected for processing (5)
client/src/webpages/dashboard/mrt/ManualReviewQueuesDashboard.tsxdb/src/scripts/api-server-pg/2026.06.17T18.15.16.restrict_routing_rules_queue_fkeys.sqlserver/services/manualReviewToolService/modules/JobRouting.test.tsserver/services/manualReviewToolService/modules/QueueOperations.test.tsserver/services/manualReviewToolService/modules/QueueOperations.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- db/src/scripts/api-server-pg/2026.06.17T18.15.16.restrict_routing_rules_queue_fkeys.sql
- server/services/manualReviewToolService/modules/JobRouting.test.ts
- client/src/webpages/dashboard/mrt/ManualReviewQueuesDashboard.tsx
- server/services/manualReviewToolService/modules/QueueOperations.test.ts
Rename the routing-rules migration to a fresh timestamp since main gained a later migration (add_sepia) during the rebase, and dedupe the two new QueueOperations.test.ts cases behind a shared helper to get the file back under the 500-line lint limit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…mmit If queue.obliterate() throws after the DB delete has already committed, a retry would see numDeletedRows === 0n, skip obliterate() entirely, and silently leave orphaned Bull/Redis data behind. Wrap it in try/catch and surface the failure to the active tracing span, matching the best-effort cleanup pattern used elsewhere (e.g. UserApi.logout). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
juanmrad
left a comment
juanmrad
left a comment
There was a problem hiding this comment.
small nit. please fix merge conflicts. Otherwise LGTM
| ((e as { constraint?: string }).constraint === | ||
| 'routing_rules_destination_queue_id_fkey' || | ||
| (e as { constraint?: string }).constraint === | ||
| 'appeals_routing_rules_destination_queue_id_fkey') | ||
| ) { |
There was a problem hiding this comment.
Nit: The cast (e as { constraint?: string }).constraint is repeated twice. Consider a single narrow:
const constraint = (e as { constraint?: string }).constraint;
if (
isForeignKeyViolationError(e) &&
(constraint === 'routing_rules_destination_queue_id_fkey' ||
constraint === 'appeals_routing_rules_destination_queue_id_fkey')
)
Reads better and avoids the duplication.
|
Hi @reitblatt! Setting up an Appeals queue requires going into the settings (new settings page released in 1.0 a month ago) and say "enable appeals". When you create a queue, you should then see a checkbox that says "This is an appeals queue" and you can populate it with jobs that correspond to those. I'll double check how to do that programmatically... |
# Conflicts: # server/services/manualReviewToolService/modules/QueueOperations.test.ts
Context & Requests for Reviewers
Part of #738 (design #1).
Previously,
routing_rules.destination_queue_idandappeals_routing_rules.destination_queue_idhadON DELETE CASCADEFK constraints, so deleting a queue would silently drop any routing rules that pointed to it, breaking routing for the org without any warning.This PR:
ON DELETE RESTRICTvia a new migration (db/src/scripts/api-server-pg/2026.06.17T18.15.16.restrict_routing_rules_queue_fkeys.sql).QueueOperations.deleteManualReviewQueue, queries the blocking rule names from both tables, and throws aQueueHasDependentRoutingRulesError(HTTP 409).Note on inflight jobs: The current implementation calls
queue.obliterate({ force: true })before the DB delete, which removes all waiting, active, and delayed Bull jobs from Redis immediately. More nuanced handling of inflight jobs (e.g. draining rather than obliterating) is tracked in #738 and will be addressed in a follow-up PR.Tests
QueueOperations.test.ts— one forrouting_rulesand one forappeals_routing_rules— verify thatdeleteManualReviewQueuerejects withQueueHasDependentRoutingRulesErrorwhen a rule references the target queue.Note: I did not test the logic for appeals rules because I couldn't figure out how to set one up. Pointers welcome!
(Optional) Rollout Plan
The migration adds
ON DELETE RESTRICTconstraints in place of the existingON DELETE CASCADEones. No data migration is needed. The constraint change is safe to apply to a live database — it only affects future deletes, not existing rows.Summary by CodeRabbit
New Features
Bug Fixes