feat(cancell-job-on-delete): Cancel queue job after file deletion in db#1058
feat(cancell-job-on-delete): Cancel queue job after file deletion in db#1058MohdShoaib-18169 wants to merge 2 commits intomainfrom
Conversation
MohdShoaib-18169
requested review from
devesh-juspay,
junaid-shirur,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an internal cancelProcessingJobs utility and integrates it into collection/folder/file creation and deletion flows, assigning singleton keys (collection_, folder_, file_) to queued processing jobs and cancelling related jobs when items or collections are deleted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Client
participant K as KnowledgeBase API
participant DB as Database
participant Q as Processing Queues
rect rgba(230,245,255,0.6)
note over U,K: Delete collection or items
U->>K: DeleteCollection/DeleteItem request
K->>DB: Delete records (collection/items)
K->>Q: cancelProcessingJobs(items, collectionId?)
note over K,Q: Cancels file, PDF, and collection jobs (singleton keys)
K-->>U: Deletion success
end
sequenceDiagram
autonumber
participant U as Client
participant K as KnowledgeBase API
participant DB as Database
participant Q as Processing Queues
rect rgba(240,255,240,0.6)
note over U,K: Create and upload flows
U->>K: CreateCollection/CreateFolder/UploadFiles
K->>DB: Persist collection/folder/file
K->>Q: Enqueue job with singletonKey (collection_<id>/folder_<id>/file_<id>)
note over K,Q: Singleton keys enable targeted cancellation later
Q-->>K: Job accepted
K-->>U: Ack with job refs
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)server/api/knowledgeBase.ts (1)
🔇 Additional comments (5)
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 |
Summary of ChangesHello @MohdShoaib-18169, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a crucial enhancement to resource management by ensuring that background processing jobs are automatically canceled when their associated files, folders, or collections are deleted from the database. This prevents the system from performing unnecessary work on non-existent items, thereby improving overall efficiency and reducing computational overhead. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to cancel pending background processing jobs when files, folders, or collections are deleted. This is achieved by adding a singletonKey to jobs upon creation and then using this key to cancel them upon deletion via a new cancelProcessingJobs utility function. The changes are logical and well-placed. My review includes suggestions to improve the implementation of the new utility function for better code clarity and robustness.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/api/knowledgeBase.ts (2)
111-127: Simplify error handling.The outer
try-catchblocks (lines 125-127 and later 134-136) are redundant since each promise already has.catch(() => {})handling. The outer try-catch won't catch anything because the inner catch prevents propagation.Apply this diff to remove redundant error handling:
...itemsToDelete.map(async (item) => { - try { const keyPrefix = item.type === "file" ? "file" : "folder" const singletonKey = `${keyPrefix}_${item.id}` if (item.type === "file") { // For files, try both queues await Promise.all([ boss.cancel(FileProcessingQueue, singletonKey).catch(() => {}), boss.cancel(PdfFileProcessingQueue, singletonKey).catch(() => {}) ]) } else { // For folders, only file-processing queue await boss.cancel(FileProcessingQueue, singletonKey).catch(() => {}) } - } catch (error) { - // Ignore errors if job doesn't exist - } }),Apply the same simplification to the collection cancellation block (lines 130-137).
101-105: Add explicit type for better clarity.The function parameters accept
itemsToDeleteas a generic array withidandtype, but in practice it receivesCollectionItem[]objects. Consider making this explicit for better type safety and documentation.async function cancelProcessingJobs(params: { - itemsToDelete: Array<{ id: string; type: string }> + itemsToDelete: Array<Pick<CollectionItem, 'id' | 'type'>> collectionId?: string userEmail: string }) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/api/knowledgeBase.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/api/knowledgeBase.ts (1)
server/queue/api-server-queue.ts (3)
boss(9-12)FileProcessingQueue(14-14)PdfFileProcessingQueue(15-15)
🔇 Additional comments (4)
server/api/knowledgeBase.ts (4)
100-149: Well-structured cancellation utility.The implementation correctly:
- Handles both file and folder types appropriately
- Attempts cancellation from both queues for files (safe even if only one was used)
- Includes collection cancellation when provided
- Uses parallel cancellation with Promise.all for efficiency
- Provides good observability through logging
The best-effort error handling approach (catching and ignoring errors) is appropriate for this use case where cancellation failures shouldn't block deletion operations.
362-362: Consistent singletonKey implementation across all enqueue operations.The singletonKey additions follow a consistent pattern:
collection_<id>for collectionsfolder_<id>for foldersfile_<id>for filesThis enables targeted cancellation and covers all job enqueue locations in the file. The format matches what the
cancelProcessingJobsfunction expects.Also applies to: 929-929, 1080-1080, 1467-1467
724-729: Proper integration of job cancellation in collection deletion.The cancellation is correctly placed after the database transaction commits (line 722) but before external resource cleanup (line 731+). This ensures:
- Database changes are persisted first
- Jobs are canceled to prevent processing deleted items
- External cleanup proceeds regardless of cancellation success
Passing both
collectionItemsToDeleteandcollectionIdcorrectly cancels jobs for all items and the collection itself.
1655-1659: Correct job cancellation for item deletion.The cancellation follows the same pattern as collection deletion - after DB commit, before external cleanup. Appropriately omits
collectionIdsince we're only deleting items, not the collection itself.The
itemsToDeletearray correctly includes both the target item and all descendants (if it's a folder), ensuring all related jobs are canceled.
1 participant
Description
Testing
Additional Notes
Summary by CodeRabbit