fix(indexing): delete vectors before catalog rows - #671
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe vector-store API now supports asynchronous filter-based deletion. Partition and file deletion cancel active indexing tasks, remove vectors before relational records, and perform follow-up cleanup. Indexing records task-scoped vector metadata and cleans vectors when catalog writes fail. ChangesCleanup and Indexing Lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WorkerDispatcher
participant TaskStateManager
participant RayTask
participant VectorStore
participant Repositories
WorkerDispatcher->>TaskStateManager: find active indexing tasks
WorkerDispatcher->>RayTask: cancel and await settlement
WorkerDispatcher->>VectorStore: delete_by_filter(partition, file_id)
WorkerDispatcher->>Repositories: remove file records
WorkerDispatcher->>VectorStore: retry filter cleanup
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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/unit/services/orchestrators/test_partition_service.py`:
- Around line 116-124: Update query_chunks_by_filter to match filters using the
same row.get(key) equality logic as delete_by_filter, so rows missing a filtered
key only match when its value is None; preserve the existing filter recording
and result structure.
🪄 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
Run ID: 443b784a-fbfe-46ed-a357-63aab0365fc6
📒 Files selected for processing (7)
openrag/core/vector_stores/vector_store.pyopenrag/services/orchestrators/partition_service.pyopenrag/services/workers/dispatcher.pytests/unit/conftest.pytests/unit/services/orchestrators/test_partition_service.pytests/unit/services/workers/stages/test_pipeline_stages.pytests/unit/services/workers/test_dispatcher.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/services/orchestrators/test_partition_service.py (1)
400-401: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
@pytest.mark.asynciodecorators
asyncio_mode = "auto"already covers these tests, so the markers can be dropped for consistency.🤖 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 `@tests/unit/services/orchestrators/test_partition_service.py` around lines 400 - 401, Remove the redundant `@pytest.mark.asyncio` decorators from test_list_all_chunks_without_file_id_filters_partition_only and the async tests at tests/unit/services/orchestrators/test_partition_service.py lines 417-418 and 425-426; asyncio_mode = "auto" already runs these tests asynchronously.
🤖 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.
Nitpick comments:
In `@tests/unit/services/orchestrators/test_partition_service.py`:
- Around line 400-401: Remove the redundant `@pytest.mark.asyncio` decorators from
test_list_all_chunks_without_file_id_filters_partition_only and the async tests
at tests/unit/services/orchestrators/test_partition_service.py lines 417-418 and
425-426; asyncio_mode = "auto" already runs these tests asynchronously.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf798b69-23ec-4dc4-8891-42e49fa9637d
📒 Files selected for processing (1)
tests/unit/services/orchestrators/test_partition_service.py
b300854 to
021aada
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 021aadad0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
021aada to
f5ff42a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81f719b9c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f4c58390c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
af22c77 to
d4955fa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4955fadcf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 140a054960
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3592a2d5c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
openrag/services/workers/task_cancellation.py (1)
32-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind structured cleanup context to the logger.
openrag/services/workers/task_cancellation.py#L32-L36: bind partition/file context before logging.openrag/services/workers/task_cancellation.py#L57-L63: bind task and cancellation context.openrag/services/workers/task_cancellation.py#L81-L86: bind pending task IDs and target context.openrag/services/workers/task_cancellation.py#L127-L170: bind settlement and stale-task context.openrag/services/workers/dispatcher.py#L145-L161: bind file deletion context.openrag/services/orchestrators/partition_service.py#L264-L299: bind partition cleanup context.As per coding guidelines, “Use the Loguru-based
get_logger()helper and structured context vialogger.bind(...)for application logging.”🤖 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 `@openrag/services/workers/task_cancellation.py` around lines 32 - 36, Replace direct contextual logging with the Loguru get_logger() helper and logger.bind(...) across all identified sites: bind partition/file_id in task_cancellation.py lines 32-36, task/cancellation context at lines 57-63, pending-task IDs and target at lines 81-86, and settlement/stale-task context at lines 127-170; bind file-deletion context in dispatcher.py lines 145-161 and partition-cleanup context in partition_service.py lines 264-299 before emitting logs.Source: Coding guidelines
🤖 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 `@openrag/services/workers/dispatcher.py`:
- Around line 153-161: Persist retryable cleanup work or durable cleanup debt
when the second-sweep vector deletion fails, and do not report file deletion as
fully successful until that cleanup is guaranteed; update
openrag/services/workers/dispatcher.py lines 153-161 around the post-delete
cleanup, apply the same guarantee in
openrag/services/orchestrators/partition_service.py lines 286-294 for partition
leftovers, and update tests/unit/services/workers/test_dispatcher.py lines
518-540 to assert the durable retry or incomplete status instead of
unconditional success.
- Around line 100-122: Update the submission flow around _pool.submit.remote and
_tsm.set_object_ref.remote so that when registration fails after task is
obtained, the returned worker ref is cancelled and settled before
_mark_submit_failed and re-raising. In
tests/unit/services/workers/test_dispatcher.py lines 161-194, add coverage for
successful submission followed by set_object_ref failure and assert the worker
cancellation occurs.
In `@openrag/services/workers/task_cancellation.py`:
- Around line 29-37: Update the AttributeError handling around
task_state_manager.get_matching_active_task_refs.remote to fail closed: log the
existing warning, then abort the deletion instead of returning 0. Ensure callers
receive the established failure signal or exception used by the surrounding
task-cancellation flow, preventing cleanup from proceeding without active-task
lookup.
- Around line 41-97: Make deletion cancellation an atomic file/partition fence
rather than a snapshot: in openrag/services/workers/task_cancellation.py:41-97,
request cancellation and wait until matching tasks cannot publish refs or
execute work; in openrag/services/workers/task_cancellation.py:137-164,
coordinate ref publication and worker settlement instead of treating state-only
FAILED updates as settled; in openrag/services/workers/dispatcher.py:138-150,
hold the deletion fence through vector and catalog cleanup; and in
openrag/services/orchestrators/partition_service.py:255-274, use the same fence
to reject or cancel indexing admitted during partition deletion.
---
Nitpick comments:
In `@openrag/services/workers/task_cancellation.py`:
- Around line 32-36: Replace direct contextual logging with the Loguru
get_logger() helper and logger.bind(...) across all identified sites: bind
partition/file_id in task_cancellation.py lines 32-36, task/cancellation context
at lines 57-63, pending-task IDs and target at lines 81-86, and
settlement/stale-task context at lines 127-170; bind file-deletion context in
dispatcher.py lines 145-161 and partition-cleanup context in
partition_service.py lines 264-299 before emitting logs.
🪄 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
Run ID: 033335f3-a4c3-41aa-836e-ddcef62ee8a2
📒 Files selected for processing (16)
openrag/core/vector_stores/vector_store.pyopenrag/di/container.pyopenrag/services/orchestrators/partition_service.pyopenrag/services/persistence/document_repo.pyopenrag/services/workers/dispatcher.pyopenrag/services/workers/indexer_actor.pyopenrag/services/workers/indexer_pool.pyopenrag/services/workers/stages/store.pyopenrag/services/workers/task_cancellation.pyopenrag/services/workers/task_state.pytests/unit/conftest.pytests/unit/services/orchestrators/test_partition_service.pytests/unit/services/persistence/test_add_file_to_partition_user_id.pytests/unit/services/workers/stages/test_pipeline_stages.pytests/unit/services/workers/test_dispatcher.pytests/unit/services/workers/test_indexer_worker.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/conftest.py
- openrag/core/vector_stores/vector_store.py
- tests/unit/services/orchestrators/test_partition_service.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2a70ce413
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f2a70ce to
460da37
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e8dd7b5e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openrag/services/workers/task_cancellation.py (1)
41-90: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftMake cancellation an atomic deletion fence.
The lookup only drains tasks visible at that instant. A newly admitted task can write after either sweep; additionally, a ref-less task marked
FAILEDcan later publish its ref becauseset_object_refacceptsFAILED, allowing it to continue while deletion proceeds.
openrag/services/workers/task_cancellation.py#L41-L90: hold a partition/file cancellation fence through cleanup and mark ref-less tasks with a registration-rejecting cancellation state.openrag/services/workers/task_state.py#L92-L96: ensure tasks cancelled for deletion cannot later publish an accepted worker ref.openrag/services/workers/dispatcher.py#L178-L195: hold the file fence across cancellation, vector sweeps, and catalog removal; indexing admission must honor it.openrag/services/orchestrators/partition_service.py#L260-L293: hold the partition fence through both vector sweeps and relational deletion.🤖 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 `@openrag/services/workers/task_cancellation.py` around lines 41 - 90, Make deletion cancellation an atomic fence across all affected sites: in openrag/services/workers/task_cancellation.py:41-90, hold the partition/file fence through both cancellation sweeps and mark ref-less tasks with a cancellation state that rejects registration; in openrag/services/workers/task_state.py:92-96, update set_object_ref to reject refs for that state; in openrag/services/workers/dispatcher.py:178-195, hold the file fence across cancellation, vector cleanup, and catalog removal while making indexing admission honor it; in openrag/services/orchestrators/partition_service.py:260-293, hold the partition fence through both vector sweeps and relational deletion.
🤖 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 `@openrag/services/workers/dispatcher.py`:
- Around line 146-176: The _cancel_submitted_task method currently returns after
cancellation errors or settlement timeouts, allowing dispatch failure to be
recorded while the worker may still run. Change the handling in
_cancel_submitted_task so cancellation and settlement must be confirmed before
returning successfully; propagate the failure or retry and fail closed when
ray.cancel fails, call_ray_actor_with_timeout times out, or settlement otherwise
cannot be confirmed.
In `@openrag/services/workers/task_cancellation.py`:
- Around line 64-89: Keep the final recheck in the surrounding cancellation flow
bounded by the original caller deadline rather than creating the independent
_STATE_UPDATE_TIMEOUT budget. Pass the remaining caller time to the final
call_ray_actor_with_timeout and _cancel_refs operations, and ensure
_mark_ref_less_tasks_failed also cannot run beyond that same absolute deadline;
preserve immediate return when no references remain.
---
Duplicate comments:
In `@openrag/services/workers/task_cancellation.py`:
- Around line 41-90: Make deletion cancellation an atomic fence across all
affected sites: in openrag/services/workers/task_cancellation.py:41-90, hold the
partition/file fence through both cancellation sweeps and mark ref-less tasks
with a cancellation state that rejects registration; in
openrag/services/workers/task_state.py:92-96, update set_object_ref to reject
refs for that state; in openrag/services/workers/dispatcher.py:178-195, hold the
file fence across cancellation, vector cleanup, and catalog removal while making
indexing admission honor it; in
openrag/services/orchestrators/partition_service.py:260-293, hold the partition
fence through both vector sweeps and relational deletion.
🪄 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
Run ID: 92216fa2-6460-48b2-824f-54ec3694ec1e
📒 Files selected for processing (20)
openrag/core/vector_stores/vector_store.pyopenrag/di/container.pyopenrag/services/orchestrators/partition_service.pyopenrag/services/persistence/document_repo.pyopenrag/services/storage/vector_store_searcher.pyopenrag/services/workers/dispatcher.pyopenrag/services/workers/indexer_actor.pyopenrag/services/workers/indexer_pool.pyopenrag/services/workers/pipeline_builder.pyopenrag/services/workers/stages/store.pyopenrag/services/workers/task_cancellation.pyopenrag/services/workers/task_state.pytests/unit/conftest.pytests/unit/services/orchestrators/test_partition_service.pytests/unit/services/persistence/test_add_file_to_partition_user_id.pytests/unit/services/storage/test_vector_store_searcher.pytests/unit/services/workers/stages/test_pipeline_stages.pytests/unit/services/workers/test_dispatcher.pytests/unit/services/workers/test_indexer_worker.pytests/unit/services/workers/test_task_state.py
🚧 Files skipped from review as they are similar to previous changes (12)
- openrag/services/workers/indexer_pool.py
- tests/unit/conftest.py
- openrag/services/persistence/document_repo.py
- openrag/core/vector_stores/vector_store.py
- openrag/services/workers/stages/store.py
- tests/unit/services/workers/stages/test_pipeline_stages.py
- openrag/di/container.py
- tests/unit/services/persistence/test_add_file_to_partition_user_id.py
- tests/unit/services/workers/test_indexer_worker.py
- tests/unit/services/orchestrators/test_partition_service.py
- openrag/services/workers/indexer_actor.py
- tests/unit/services/workers/test_dispatcher.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 259a333ab5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 716858bc89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f64a7ade27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f64a7ad to
b19a96e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b19a96e58b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b5efc42db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6c4d90aad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f6c4d90 to
fb3cb44
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8daf6a2957
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dca53abc39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
dca53ab to
f6b8162
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6b8162860
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f6b8162 to
4649df8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4649df8ceb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4649df8 to
5e1dd60
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e1dd607da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
5e1dd60 to
6fc790a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fc790aea8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
There was a problem hiding this comment.
Reviewed end-to-end across multiple rounds as the branch evolved. Summary of what I verified directly against the code (not just bot self-reports, several of which turned out to reference commits no longer reachable after rebases):
- Core fix (vectors deleted before catalog rows, both file and partition paths) is correct.
- Task-id-scoped chunk stamping/cleanup for partial-failure and replace-cleanup ordering is correct — traced the row-mutation-by-reference behavior that makes cleanup work even when the pipeline raises mid-stage.
- Internal metadata (
_openrag_indexing_task_id) stripping is now centralized incore/utils/conts.pyand applied consistently across all read paths. - The admission/delete fencing added in
6fc790ae/c9e03664is a real fix, not cosmetic: traced the Postgres advisory lock (partition level, cross-process safe) and theTaskStateManager-based reference-counted fence (file level) through both the upload and delete code paths, confirmed the lock guard correctly reuses its held connection, and confirmed test coverage for the main failure modes. - Legacy-actor rolling-deploy compatibility (
ac1455c1) degrades gracefully via exception-chain detection + retry, well tested. - CI green on
c9e03664(lint, unit, api-tests, milvus-integration).
One confirmed non-blocking finding I haven't seen addressed yet: _matching_active_task_refs_locked (task_state.py) treats every detail-less QUEUED task as a match for any delete, regardless of partition/file — this only fires in the legacy non-atomic set_state+set_details fallback path, but when it does, deleting file/partition A can spuriously delay or cancel an unrelated concurrent upload of file/partition B. Collateral-cancellation bug, not a data-integrity issue — fine as a fast follow-up rather than blocking this PR.
Approving — the primary failure mode (#658) is solidly fixed and the residual gaps are narrow, well-understood, and non-blocking.
#671 and this branch each carried their own copy of test_cancelled_state_is_not_overwritten_by_worker_transitions, with identical assertions and different manager helpers, so reconciling the two versions of the module kept both and ruff flagged the redefinition (F811). Keeps #671's copy, which the rest of that file's helpers match, and moves this branch's docstring onto it.
4 participants
Summary
Fixes the delete failure mode described in #658. File and partition deletes now clean the vector store before removing the catalog rows, so a Milvus cleanup failure does not leave users with a deleted catalog entry whose chunks are still queryable.
Why
The previous flow removed Postgres rows first and treated Milvus cleanup as best effort. If that cleanup failed, deleted content could remain searchable, and a reused partition name could expose stale chunks.
Validation
Closes #658
Summary by CodeRabbit
New Features
Bug Fixes