fix(vector_io): decrement usage_bytes on delete, exclude failed inserts - #6552
Merged
Merged
Conversation
…aving it at 0 openai_attach_file_to_vector_store initialized usage_bytes to 0 on both the VectorStoreFileObject and the parent vector store's metadata dict, but never updated it after successfully chunking, embedding, and inserting the file's content. Chunks were still indexed correctly, but usage_bytes stayed 0 indefinitely, matching ogx-ai#6542. Compute it from the UTF-8 byte length of the chunk text used for the embeddings request, set it on the file object, and add it to the parent vector store's running usage_bytes total under the existing per-store lock. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: elina priyadarshinee <elina.priyadarshinee1@ibm.com>
…nserts Two follow-ups from PR review on the usage_bytes fix (ogx-ai#6542): - openai_delete_vector_store_file now subtracts the deleted file's usage_bytes from the parent store's running total (clamped at 0 as a safety net against any prior undercounting), mirroring the increment added in openai_attach_file_to_vector_store. - openai_attach_file_to_vector_store now computes usage_bytes only after insert_chunks succeeds, instead of before the embeddings call. A failed embed/insert leaves the file status "failed" but previously still left usage_bytes computed from chunk text, which then got added to the store's total even though nothing was actually indexed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: elina priyadarshinee <elina.priyadarshinee1@ibm.com>
elinacse
requested review from
bbrowning,
cdoern,
franciscojavierarceo,
leseb,
mattf,
raghotham and
skamenan7
as code owners
September 15, 2026 11:22
…age-bytes Signed-off-by: elina priyadarshinee <elina.priyadarshinee1@ibm.com> # Conflicts: # src/ogx/providers/utils/memory/openai_vector_store_mixin.py # tests/unit/providers/utils/memory/test_openai_vector_store_mixin.py
Contributor
Author
|
@mattf please review this PR . Thanks! |
mattf
requested changes
Sep 15, 2026
mattf
left a comment
mattf
left a comment
Collaborator
There was a problem hiding this comment.
- Delete path still mutates store state without the per-store lock. openai_delete_vector_store_file does a read-modify-write of file_ids/file_counts/usage_bytes on the shallow-copied store_info and writes it back, without taking self._get_vector_store_lock(vector_store_id) — while the attach path's increment runs under that lock. The commit message says the decrement "mirrors the increment," but it mirrors the arithmetic, not the synchronization. A concurrent attach + delete (or two deletes) is a lost-update race that now also corrupts usage_bytes. Pre-existing for file_counts, but this PR extends it to the new accounting. This was also called out in the issue; worth either fixing here (wrap the update block in the lock, as attach does at ~line 1449) or a linked follow-up.
- PR body should close the issues. It doesn't reference #6550/#6551, so they'll stay open after merge. Add Closes #6550 / Closes #6551.
- Delete tests assert in-memory state only. They check mixin.openai_vector_stores[...]["usage_bytes"] but never that the persisted value (the upsert/_save_openai_vector_store call) carries the decremented total. A regression that updates memory but skips the save would pass both tests. Also, the two delete tests are ~50 lines of near-identical mock scaffolding — pytest.param would cut it in half.
…e_counts The commit that decremented usage_bytes on delete mirrored the attach path's arithmetic but not its synchronization: openai_delete_vector_store_file read store_info, did the slow chunk-deletion work, then wrote file_ids/ file_counts/usage_bytes back without holding self._get_vector_store_lock, while attach's increment runs entirely inside that lock. A concurrent attach + delete (or two deletes) racing on the same store is a lost update that can now also corrupt usage_bytes, not just file_counts. Wrap the read-modify-write in the lock and re-fetch store_info from self.openai_vector_stores fresh inside it (as attach already does), instead of mutating the copy taken before the slow work. Also consolidate the two near-identical delete usage_bytes tests into one pytest.param-ed test, and assert the persisted upsert payload carries the decremented total, not just the in-memory cache -- a regression that updates memory but skips the save would have passed both previous tests. Closes ogx-ai#6550 Closes ogx-ai#6551 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: elina priyadarshinee <elina.priyadarshinee1@ibm.com>
…-ai#6551) The fix in this branch already computes usage_bytes only after insert_chunks succeeds, so a failed openai_embeddings call (not just a failed insert_chunks) correctly leaves it at 0 -- but only the insert_chunks failure path had a regression test. Parametrize it to cover both failure points ogx-ai#6551 calls out explicitly, closing the gap in test coverage rather than the behavior, which was already correct. Closes ogx-ai#6551 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: elina priyadarshinee <elina.priyadarshinee1@ibm.com>
Contributor
Author
|
@mattf Please review it again. Thanks ! |
mattf
approved these changes
Sep 16, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Sep 17, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Sep 17, 2026
Contributor
|
Recording workflow completed Providers: fireworks Recordings have been generated and will be committed automatically by the companion workflow. Fork PR: Recordings will be committed if you have "Allow edits from maintainers" enabled. |
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-Authored-By: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Contributor
|
✅ Recordings committed successfully Recordings from the integration tests have been committed to this PR. |
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Sep 17, 2026
…ts --inference-mode record-if-missing --setup fireworks --suite base --install-deps --pattern test_mcp_tools_in_inference --client-version latest
mattf
enabled auto-merge
September 17, 2026 18:56
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.
What does this PR do?
decrement in openai_delete_vector_store_file
avoid adding to the usage count when insert_chunks fails
closes #6550
closes #6551