fix(workspaces): exclude indexed files from orphan check on workspace delete - #285
Merged
Conversation
… delete delete_workspace() determined orphaned files by checking whether a file appeared in any other workspace. It did not check the files table, so any file that was independently indexed into the partition and then added to a workspace would be permanently deleted from Milvus when that workspace was removed. Fix: add a second exclusion subquery against the files table so that only files that are truly workspace-only (not independently indexed and not in any other workspace) are returned as orphans for deletion.
Cover: independently-indexed file not returned as orphan, workspace-only file returned as orphan, file shared across workspaces not returned, mixed scenario where only the true orphan is returned, empty workspace.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdated delete_workspace logic to avoid deleting files that are independently indexed by excluding Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Utils as "delete_workspace()"
participant DB as "Database (Workspace, WorkspaceFile, File)"
Caller->>Utils: delete_workspace(workspace_id)
Utils->>DB: SELECT Workspace WHERE id=workspace_id
alt workspace exists
Utils->>DB: SELECT WorkspaceFile.file_id WHERE workspace_id == target
Utils->>DB: SELECT WorkspaceFile.file_id WHERE workspace_id != target (subq_other_ws)
Utils->>DB: SELECT File.file_id WHERE partition_name == target.partition (subq_indexed)
Utils->>Utils: compute orphan_ids = target_files - subq_other_ws - subq_indexed
Utils->>DB: DELETE FROM WorkspaceFile WHERE workspace_id == target
Utils->>DB: DELETE FROM Workspace WHERE id == target
Utils->>DB: COMMIT
Utils->>Caller: return orphan_ids
else workspace missing
Utils->>Caller: return []
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
🧹 Nitpick comments (2)
openrag/components/indexer/vectordb/test_delete_workspace.py (1)
142-154: Consider adding a cross-partition test case.The mixed scenario test is comprehensive, but all tests use a single partition "p1". Consider adding a test case with files in multiple partitions to verify the
subq_indexedbehavior doesn't incorrectly exclude orphans due to same-named file_ids in different partitions.💡 Optional: Cross-partition test case
def test_file_indexed_in_different_partition_does_not_affect_orphan(db): """A file_id indexed in partition p2 should not prevent orphan detection in p1.""" db.add_file("p2", "file-x") # Indexed in different partition db.add_workspace("ws1", "p1") db.add_file_to_workspace("ws1", "file-x") # Same file_id, but not indexed in p1 orphans = db.delete_workspace("ws1") # If file_ids are globally unique (UUIDs), this would return [] # If file_ids can be reused across partitions, this should return ["file-x"] # Update assertion based on expected behavior assert orphans == [] # or ["file-x"] depending on design decision🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/indexer/vectordb/test_delete_workspace.py` around lines 142 - 154, Add a cross-partition unit test to ensure delete_workspace's orphan detection doesn't treat a file indexed in another partition as indexed in this partition: create a new test (e.g., test_file_indexed_in_different_partition_does_not_affect_orphan) that calls db.add_file("p2", "file-x"), db.add_workspace("ws1", "p1"), db.add_file_to_workspace("ws1", "file-x"), then call db.delete_workspace("ws1") and assert the expected orphans list based on your partitioning semantics; this will exercise the same subquery/logic used by delete_workspace and subq_indexed to verify partition-scoped indexing behavior.openrag/components/indexer/vectordb/utils.py (1)
636-660: LGTM! The orphan detection logic correctly addresses the data loss issue.The fix properly excludes both files in other workspaces and independently-indexed files from the orphan set, which aligns with the PR objectives for issue
#275.Minor optional simplification: The subqueries can be used directly without wrapping in another
select():♻️ Optional: Simplify subquery usage
with self.Session() as session: - # Files present in at least one other workspace - subq_other_ws = select(WorkspaceFile.file_id).where(WorkspaceFile.workspace_id != workspace_id).subquery() - # Files that were independently indexed (present in the files table) - subq_indexed = select(File.file_id).subquery() + # Files present in at least one other workspace + subq_other_ws = select(WorkspaceFile.file_id).where(WorkspaceFile.workspace_id != workspace_id) + # Files that were independently indexed (present in the files table) + subq_indexed = select(File.file_id) result = session.execute( select(WorkspaceFile.file_id) .where(WorkspaceFile.workspace_id == workspace_id) - .where(WorkspaceFile.file_id.notin_(select(subq_other_ws.c.file_id))) - .where(WorkspaceFile.file_id.notin_(select(subq_indexed.c.file_id))) + .where(WorkspaceFile.file_id.notin_(subq_other_ws)) + .where(WorkspaceFile.file_id.notin_(subq_indexed)) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/indexer/vectordb/utils.py` around lines 636 - 660, The current delete_workspace implementation builds subq_other_ws and subq_indexed then wraps them in extra select(...) calls when using .notin_; simplify by passing the subquery objects directly to the .notin_ predicates. Update the .where clauses in delete_workspace that reference WorkspaceFile.file_id.notin_(select(subq_other_ws.c.file_id)) and WorkspaceFile.file_id.notin_(select(subq_indexed.c.file_id)) to use WorkspaceFile.file_id.notin_(subq_other_ws) and WorkspaceFile.file_id.notin_(subq_indexed) respectively, keeping the same semantics and leaving other logic (session handling, deletion, commit) untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openrag/components/indexer/vectordb/utils.py`:
- Around line 647-648: subq_indexed currently selects all File.file_id without
partition filtering which breaks orphan detection across partitions; update the
logic to fetch the target Workspace's partition (from the Workspace row used in
this routine) and restrict subq_indexed to that partition (e.g. replace
select(File.file_id).subquery() with select(File.file_id).where(File.partition
== workspace_partition).subquery()), ensuring you reference the
Workspace.partition field and the unique constraint uix_file_id_partition when
applying the filter.
---
Nitpick comments:
In `@openrag/components/indexer/vectordb/test_delete_workspace.py`:
- Around line 142-154: Add a cross-partition unit test to ensure
delete_workspace's orphan detection doesn't treat a file indexed in another
partition as indexed in this partition: create a new test (e.g.,
test_file_indexed_in_different_partition_does_not_affect_orphan) that calls
db.add_file("p2", "file-x"), db.add_workspace("ws1", "p1"),
db.add_file_to_workspace("ws1", "file-x"), then call db.delete_workspace("ws1")
and assert the expected orphans list based on your partitioning semantics; this
will exercise the same subquery/logic used by delete_workspace and subq_indexed
to verify partition-scoped indexing behavior.
In `@openrag/components/indexer/vectordb/utils.py`:
- Around line 636-660: The current delete_workspace implementation builds
subq_other_ws and subq_indexed then wraps them in extra select(...) calls when
using .notin_; simplify by passing the subquery objects directly to the .notin_
predicates. Update the .where clauses in delete_workspace that reference
WorkspaceFile.file_id.notin_(select(subq_other_ws.c.file_id)) and
WorkspaceFile.file_id.notin_(select(subq_indexed.c.file_id)) to use
WorkspaceFile.file_id.notin_(subq_other_ws) and
WorkspaceFile.file_id.notin_(subq_indexed) respectively, keeping the same
semantics and leaving other logic (session handling, deletion, commit)
untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 988c052a-220c-47ef-9cef-f827b27edf5f
📒 Files selected for processing (2)
openrag/components/indexer/vectordb/test_delete_workspace.pyopenrag/components/indexer/vectordb/utils.py
…workspace subq_indexed was selecting all file_ids from the files table without filtering by partition. Since file_id is only unique per (file_id, partition_name), the same filename in a different partition could incorrectly prevent a file from being identified as orphaned. Now fetches the workspace's partition_name and filters File.file_id to that partition only. Also adds a cross-partition regression test.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openrag/components/indexer/vectordb/test_delete_workspace.py (1)
166-171: Consider adding a test for non-existent workspace.The helper's
delete_workspacereturns[]when the workspace doesn't exist (lines 80-81), mirroring the production early-return behavior. Adding explicit coverage would be a good-to-have:def test_nonexistent_workspace_returns_empty_list(db): """Deleting a workspace that doesn't exist returns an empty list.""" orphans = db.delete_workspace("nonexistent-ws") assert orphans == []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/indexer/vectordb/test_delete_workspace.py` around lines 166 - 171, Add a new unit test in openrag/components/indexer/vectordb/test_delete_workspace.py that calls the helper method delete_workspace with a workspace id that was never created and asserts it returns an empty list; name the test test_nonexistent_workspace_returns_empty_list and simply call orphans = db.delete_workspace("nonexistent-ws") followed by assert orphans == [] to cover the early-return behavior in delete_workspace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openrag/components/indexer/vectordb/test_delete_workspace.py`:
- Around line 166-171: Add a new unit test in
openrag/components/indexer/vectordb/test_delete_workspace.py that calls the
helper method delete_workspace with a workspace id that was never created and
asserts it returns an empty list; name the test
test_nonexistent_workspace_returns_empty_list and simply call orphans =
db.delete_workspace("nonexistent-ws") followed by assert orphans == [] to cover
the early-return behavior in delete_workspace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66132cb3-0f90-46e6-8746-4bc6101b80c6
📒 Files selected for processing (2)
openrag/components/indexer/vectordb/test_delete_workspace.pyopenrag/components/indexer/vectordb/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openrag/components/indexer/vectordb/utils.py
…kspace test - Drop unnecessary .subquery() + select() wrapping in delete_workspace; pass select objects directly to .notin_() - Add test_nonexistent_workspace_returns_empty_list to cover the early-return path
This was referenced May 6, 2026
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
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.
Summary
Fixes #275.
delete_workspace()determined which files to delete from Milvus by checking whether a file appeared in any other workspace. It did not consult thefilestable, so a file that was independently indexed into the partition and then added to a workspace would be permanently deleted from Milvus when that workspace was removed — irreversible data loss.Root cause
Fix
Add a second exclusion subquery against
filesso that a file is only considered orphaned if it is both absent from every other workspace and not present in thefilestable (i.e. was never independently indexed).Filewas already imported inutils.py— no new imports needed.Tests
5 new unit tests in
test_delete_workspace.py(SQLite in-memory, no Ray):227 passed, 3 skipped (pre-existing)
Summary by CodeRabbit
Bug Fixes
Tests