diff --git a/backend/app/crud/rag/open_ai.py b/backend/app/crud/rag/open_ai.py index 0df73ab39..6d80e12c9 100644 --- a/backend/app/crud/rag/open_ai.py +++ b/backend/app/crud/rag/open_ai.py @@ -115,11 +115,16 @@ def update( ) try: - batch = self.client.vector_stores.file_batches.upload_and_poll( + created = self.client.vector_stores.file_batches.create( vector_store_id=vector_store_id, - files=[], file_ids=[doc.file_id[OPENAI_PROVIDER] for doc in docs], ) + # poll()'s return deserializes a vector-store body, so its .id is the vs_ id; + # capture the real vsfb_ batch id from create() before polling. + batch_id = created.id + batch = self.client.vector_stores.file_batches.poll( + batch_id, vector_store_id=vector_store_id + ) except openai.RateLimitError as e: error_message = ( f"[OPENAI] Rate limit exceeded (code: {e.status_code}): " @@ -215,14 +220,14 @@ def update( logger.info( f"[OpenAIVectorStoreCrud.update] Batch complete | " - f"{{'vector_store_id': '{vector_store_id}', " + f"{{'vector_store_id': '{vector_store_id}', 'batch_id': '{batch_id}', " f"'completed': {batch.file_counts.completed}, 'failed': {batch.file_counts.failed}}}" ) if batch.file_counts.failed > 0: try: failed_files = self.client.vector_stores.file_batches.list_files( vector_store_id=vector_store_id, - batch_id=batch.id, + batch_id=batch_id, filter="failed", ) doc_by_file_id = {d.file_id[OPENAI_PROVIDER]: d for d in docs} @@ -232,11 +237,15 @@ def update( label = d.fname if d else f.id msg = f.last_error.message if f.last_error else "no error detail" parts.append(f"{label}: {msg}") + logger.error( + f"[OpenAIVectorStoreCrud.update] Files failed to index | " + f"{{'batch_id': '{batch_id}', 'failed_files': '{', '.join(parts)}'}}" + ) raise RuntimeError("; ".join(parts)) except OpenAIError as err: logger.warning( f"[OpenAIVectorStoreCrud.update] Could not fetch per-file errors | " - f"{{'batch_id': '{batch.id}', 'error': '{str(err)}'}}" + f"{{'batch_id': '{batch_id}', 'error': '{str(err)}'}}" ) raise diff --git a/backend/app/tests/crud/rag/test_open_ai.py b/backend/app/tests/crud/rag/test_open_ai.py index d8c85c227..04558c28c 100644 --- a/backend/app/tests/crud/rag/test_open_ai.py +++ b/backend/app/tests/crud/rag/test_open_ai.py @@ -37,10 +37,19 @@ def docs(): ] -def _batch_result(*, completed: int, failed: int, batch_id: str = "batch_abc"): - """Mock the return of vector_stores.file_batches.upload_and_poll.""" +REAL_BATCH_ID = "vsfb_real123" +CORRUPT_POLL_ID = "vs_corrupt999" + + +def _wire_batch(mock_client, *, completed: int, failed: int) -> None: + """Stub create() with the real vsfb_ id and poll() with the corrupt vs_ id the SDK returns.""" + mock_client.vector_stores.file_batches.create.return_value = MagicMock( + id=REAL_BATCH_ID + ) counts = MagicMock(completed=completed, failed=failed) - return MagicMock(id=batch_id, file_counts=counts) + mock_client.vector_stores.file_batches.poll.return_value = MagicMock( + id=CORRUPT_POLL_ID, file_counts=counts + ) def _failed_file(file_id: str, error_message: str | None): @@ -53,30 +62,35 @@ def _failed_file(file_id: str, error_message: str | None): class TestOpenAIVectorStoreCrudUpdateSuccess: def test_completes_when_all_files_complete(self, crud, mock_client, docs): - mock_client.vector_stores.file_batches.upload_and_poll.return_value = ( - _batch_result(completed=2, failed=0) - ) + _wire_batch(mock_client, completed=2, failed=0) crud.update("vs_1", docs) - _, kwargs = mock_client.vector_stores.file_batches.upload_and_poll.call_args + _, kwargs = mock_client.vector_stores.file_batches.create.call_args assert kwargs["vector_store_id"] == "vs_1" assert kwargs["file_ids"] == ["file-1", "file-2"] # list_files should not have been called on the happy path mock_client.vector_stores.file_batches.list_files.assert_not_called() + def test_polls_with_the_create_batch_id(self, crud, mock_client, docs): + _wire_batch(mock_client, completed=2, failed=0) + + crud.update("vs_1", docs) + + args, kwargs = mock_client.vector_stores.file_batches.poll.call_args + assert args[0] == REAL_BATCH_ID + assert kwargs["vector_store_id"] == "vs_1" + def test_skips_upload_when_no_docs(self, crud, mock_client): crud.update("vs_1", []) - mock_client.vector_stores.file_batches.upload_and_poll.assert_not_called() + mock_client.vector_stores.file_batches.create.assert_not_called() class TestOpenAIVectorStoreCrudUpdatePartialFailure: """Failed files -> RuntimeError with per-file reasons labelled by fname.""" def test_includes_failed_fnames_and_messages(self, crud, mock_client, docs): - mock_client.vector_stores.file_batches.upload_and_poll.return_value = ( - _batch_result(completed=1, failed=1) - ) + _wire_batch(mock_client, completed=1, failed=1) mock_client.vector_stores.file_batches.list_files.return_value = [ _failed_file("file-1", "Unsupported file type"), _failed_file("file-2", "File too large"), @@ -89,15 +103,29 @@ def test_includes_failed_fnames_and_messages(self, crud, mock_client, docs): assert "file1.pdf: Unsupported file type" in msg assert "file2.pdf: File too large" in msg + def test_looks_up_failures_with_create_batch_id_not_poll_id( + self, crud, mock_client, docs + ): + """Regression: poll()'s return .id is the vs_ id, which list_files rejects.""" + _wire_batch(mock_client, completed=1, failed=1) + mock_client.vector_stores.file_batches.list_files.return_value = [ + _failed_file("file-1", "Unsupported file type") + ] + + with pytest.raises(RuntimeError): + crud.update("vs_1", docs) + + _, kwargs = mock_client.vector_stores.file_batches.list_files.call_args + assert kwargs["batch_id"] == REAL_BATCH_ID + assert kwargs["batch_id"] != CORRUPT_POLL_ID + def test_reports_no_error_detail_when_last_error_missing( self, crud, mock_client, docs ): """A failed file with no `last_error` shouldn't drop out of the summary — it gets 'no error detail' so the user sees that something was wrong with that file even if OpenAI didn't tell us what.""" - mock_client.vector_stores.file_batches.upload_and_poll.return_value = ( - _batch_result(completed=1, failed=1) - ) + _wire_batch(mock_client, completed=1, failed=1) mock_client.vector_stores.file_batches.list_files.return_value = [ _failed_file("file-1", None) ] @@ -109,9 +137,7 @@ def test_falls_back_to_file_id_label_for_unknown_file( self, crud, mock_client, docs ): """A failed file ID not matching any doc is labelled by its file ID.""" - mock_client.vector_stores.file_batches.upload_and_poll.return_value = ( - _batch_result(completed=1, failed=1) - ) + _wire_batch(mock_client, completed=1, failed=1) mock_client.vector_stores.file_batches.list_files.return_value = [ _failed_file("file-unknown", "parse error") ] @@ -122,9 +148,7 @@ def test_falls_back_to_file_id_label_for_unknown_file( def test_reraises_when_list_files_errors(self, crud, mock_client, docs): """If the follow-up list_files lookup itself raises, the OpenAI error propagates instead of masking the real upload problem.""" - mock_client.vector_stores.file_batches.upload_and_poll.return_value = ( - _batch_result(completed=0, failed=2) - ) + _wire_batch(mock_client, completed=0, failed=2) mock_client.vector_stores.file_batches.list_files.side_effect = ( openai.OpenAIError("list failed") ) @@ -134,7 +158,7 @@ def test_reraises_when_list_files_errors(self, crud, mock_client, docs): class TestOpenAIVectorStoreCrudUpdateOpenAIExceptions: - """`upload_and_poll` raising each specific OpenAI exception type maps to + """`create` raising each specific OpenAI exception type maps to `InterruptedError` with a category-prefixed message that includes the upstream status code and a remediation hint. @@ -230,9 +254,7 @@ def test_specific_openai_exception_maps_to_category_prefix( expected_status, original_message, ): - mock_client.vector_stores.file_batches.upload_and_poll.side_effect = ( - exception_factory() - ) + mock_client.vector_stores.file_batches.create.side_effect = exception_factory() with pytest.raises(InterruptedError) as exc_info: crud.update("vs_1", docs) @@ -243,7 +265,7 @@ def test_specific_openai_exception_maps_to_category_prefix( def test_api_timeout_error(self, crud, mock_client, docs): """APITimeoutError doesn't expose .message — handler interpolates str(e).""" - mock_client.vector_stores.file_batches.upload_and_poll.side_effect = ( + mock_client.vector_stores.file_batches.create.side_effect = ( openai.APITimeoutError(request=MagicMock()) ) @@ -256,8 +278,8 @@ def test_generic_openai_error_falls_through(self, crud, mock_client, docs): bottom-most `except openai.OpenAIError` block — prefixed with the generic "OpenAI error" tag but still carrying the original message. """ - mock_client.vector_stores.file_batches.upload_and_poll.side_effect = ( - openai.OpenAIError("something else") + mock_client.vector_stores.file_batches.create.side_effect = openai.OpenAIError( + "something else" ) with pytest.raises(InterruptedError) as exc_info: diff --git a/backend/app/tests/services/collections/providers/test_openai_provider.py b/backend/app/tests/services/collections/providers/test_openai_provider.py index 1c07c100f..9911bf133 100644 --- a/backend/app/tests/services/collections/providers/test_openai_provider.py +++ b/backend/app/tests/services/collections/providers/test_openai_provider.py @@ -380,11 +380,13 @@ def test_upload_files_first_failure_stops_remaining_docs() -> None: # --------------------------------------------------------------------------- -def _make_batch(completed: int, failed: int) -> MagicMock: - batch = MagicMock() +def _wire_batch(client: MagicMock, completed: int, failed: int) -> None: + """create() yields the real vsfb_ id; poll()'s return carries the corrupt vs_ id.""" + client.vector_stores.file_batches.create.return_value = MagicMock(id="vsfb_real") + batch = MagicMock(id="vs_corrupt") batch.file_counts.completed = completed batch.file_counts.failed = failed - return batch + client.vector_stores.file_batches.poll.return_value = batch def _make_openai_doc(file_id: str = "file-abc", fname: str = "doc.pdf") -> MagicMock: @@ -398,24 +400,20 @@ def test_vector_store_update_skips_when_no_docs() -> None: client = MagicMock() crud = OpenAIVectorStoreCrud(client) crud.update("vs_123", []) - client.vector_stores.file_batches.upload_and_poll.assert_not_called() + client.vector_stores.file_batches.create.assert_not_called() def test_vector_store_update_succeeds_with_no_failures() -> None: client = MagicMock() - client.vector_stores.file_batches.upload_and_poll.return_value = _make_batch( - completed=3, failed=0 - ) + _wire_batch(client, completed=3, failed=0) crud = OpenAIVectorStoreCrud(client) crud.update("vs_123", [_make_openai_doc() for _ in range(3)]) - client.vector_stores.file_batches.upload_and_poll.assert_called_once() + client.vector_stores.file_batches.create.assert_called_once() def test_vector_store_update_raises_on_openai_error() -> None: client = MagicMock() - client.vector_stores.file_batches.upload_and_poll.side_effect = OpenAIError( - "rate limit" - ) + client.vector_stores.file_batches.create.side_effect = OpenAIError("rate limit") crud = OpenAIVectorStoreCrud(client) with pytest.raises(InterruptedError, match="rate limit"): @@ -430,9 +428,7 @@ def _make_failed_file(message: str) -> MagicMock: def test_vector_store_update_raises_on_partial_failures() -> None: client = MagicMock() - client.vector_stores.file_batches.upload_and_poll.return_value = _make_batch( - completed=2, failed=1 - ) + _wire_batch(client, completed=2, failed=1) client.vector_stores.file_batches.list_files.return_value = [ _make_failed_file("unsupported file type") ] @@ -444,9 +440,7 @@ def test_vector_store_update_raises_on_partial_failures() -> None: def test_vector_store_update_raises_on_all_failures() -> None: client = MagicMock() - client.vector_stores.file_batches.upload_and_poll.return_value = _make_batch( - completed=0, failed=2 - ) + _wire_batch(client, completed=0, failed=2) client.vector_stores.file_batches.list_files.return_value = [ _make_failed_file("invalid pdf"), _make_failed_file("parse error"), @@ -459,15 +453,13 @@ def test_vector_store_update_raises_on_all_failures() -> None: def test_vector_store_update_passes_file_ids_to_openai() -> None: client = MagicMock() - client.vector_stores.file_batches.upload_and_poll.return_value = _make_batch( - completed=2, failed=0 - ) + _wire_batch(client, completed=2, failed=0) crud = OpenAIVectorStoreCrud(client) docs = [_make_openai_doc("file-1"), _make_openai_doc("file-2")] crud.update("vs_abc", docs) - _, kwargs = client.vector_stores.file_batches.upload_and_poll.call_args + _, kwargs = client.vector_stores.file_batches.create.call_args assert kwargs["vector_store_id"] == "vs_abc" assert kwargs["file_ids"] == ["file-1", "file-2"] diff --git a/backend/app/tests/utils/llm_provider.py b/backend/app/tests/utils/llm_provider.py index 564f27984..df135141c 100644 --- a/backend/app/tests/utils/llm_provider.py +++ b/backend/app/tests/utils/llm_provider.py @@ -116,11 +116,12 @@ def get_mock_openai_client_with_vector_store() -> MagicMock: mock_client.vector_stores.create.return_value = mock_vector_store mock_file_batch = MagicMock() + mock_file_batch.id = "vsfb_mock" mock_file_batch.file_counts.completed = 2 mock_file_batch.file_counts.total = 2 - mock_client.vector_stores.file_batches.upload_and_poll.return_value = ( - mock_file_batch - ) + mock_file_batch.file_counts.failed = 0 + mock_client.vector_stores.file_batches.create.return_value = mock_file_batch + mock_client.vector_stores.file_batches.poll.return_value = mock_file_batch mock_client.vector_stores.files.list.return_value = {"data": []} diff --git a/backend/app/tests/utils/openai.py b/backend/app/tests/utils/openai.py index 4cceab033..093f45ae6 100644 --- a/backend/app/tests/utils/openai.py +++ b/backend/app/tests/utils/openai.py @@ -116,13 +116,14 @@ def get_mock_openai_client_with_vector_store() -> MagicMock: mock_vector_store.id = "mock_vector_store_id" mock_client.vector_stores.create.return_value = mock_vector_store - # File upload + polling + # File batch creation + polling mock_file_batch = MagicMock() + mock_file_batch.id = "vsfb_mock" mock_file_batch.file_counts.completed = 2 mock_file_batch.file_counts.total = 2 - mock_client.vector_stores.file_batches.upload_and_poll.return_value = ( - mock_file_batch - ) + mock_file_batch.file_counts.failed = 0 + mock_client.vector_stores.file_batches.create.return_value = mock_file_batch + mock_client.vector_stores.file_batches.poll.return_value = mock_file_batch # File list mock_client.vector_stores.files.list.return_value = {"data": []} diff --git a/docs/wiki/modules/knowledge-base.md b/docs/wiki/modules/knowledge-base.md index 4307e6b30..1582afa74 100644 --- a/docs/wiki/modules/knowledge-base.md +++ b/docs/wiki/modules/knowledge-base.md @@ -35,3 +35,4 @@ All paths relative to `backend/app/`. ## Gotchas - Uploads de-duplicate by provider file ID (see deep dive §7). - Collections are immutable-ish: deletion semantics in deep dive §10. +- OpenAI file-batch id: the SDK's `file_batches.poll()` / `upload_and_poll()` final return deserializes a vector-store body, so its `.id` is the `vs_` id, not the `vsfb_` batch id. `crud/rag/open_ai.py` captures the batch id from `create()` before polling and uses it for `list_files`. Any failed file is a hard failure (whole vector store rolled back); partial indexing needs an add-documents endpoint first.