From 8d2bd8f19f19d5cc91230f8e356fe7c0dd9bf221 Mon Sep 17 00:00:00 2001 From: Gordon YUEN Date: Sun, 9 Aug 2026 19:53:43 +0800 Subject: [PATCH] fix: skip presigning asset URLs for missing chunk artifacts _document_chunk_asset_url presigned an asset URL for every image/table chunk with a file_path, with no existence check, so chunks whose raw artifact is absent from the results bucket (e.g. old jobs with dropped files) exposed dangling 404 URLs to clients. Verify the artifact exists before presigning; return null when missing and fail closed on storage errors, so clients never receive URLs that 404. - unit tests: URL generated when file exists; null when missing; null on verification errors and for non-media chunks / include_asset_urls=false - contract tests: seed real raw assets (tables/table-1.html, images/figure-1.png) so URL assertions check actual objects; fakes gain verify_raw_exists --- .../services/documents/lifecycle_service.py | 9 + .../contract/test_demo_documents_contract.py | 3 + .../tests/contract/test_documents_contract.py | 24 +++ .../tests/contract/test_retrieval_contract.py | 3 + .../unit/test_document_chunk_asset_url.py | 177 ++++++++++++++++++ 5 files changed, 216 insertions(+) create mode 100644 apps/api/tests/unit/test_document_chunk_asset_url.py diff --git a/apps/api/app/services/documents/lifecycle_service.py b/apps/api/app/services/documents/lifecycle_service.py index 1bd5ae3a9..a0328d507 100644 --- a/apps/api/app/services/documents/lifecycle_service.py +++ b/apps/api/app/services/documents/lifecycle_service.py @@ -47,6 +47,15 @@ def _document_chunk_asset_url( return None try: + if not result_storage.verify_raw_exists( + job_id=job_id, + relative_path=file_path, + ): + logger.warning( + f"Skipping asset URL for missing chunk artifact: " + f"job_id={job_id}, file_path={file_path}" + ) + return None return result_storage.generate_artifact_url( job_id=job_id, artifact_ref=file_path, diff --git a/apps/api/tests/contract/test_demo_documents_contract.py b/apps/api/tests/contract/test_demo_documents_contract.py index a1e704747..c00bee07c 100644 --- a/apps/api/tests/contract/test_demo_documents_contract.py +++ b/apps/api/tests/contract/test_demo_documents_contract.py @@ -62,6 +62,9 @@ def normalize_artifact_ref(self, artifact_ref: str | None) -> str | None: return normalized return None + def verify_raw_exists(self, *, job_id: str, relative_path: str) -> bool: + return relative_path in self.raw_files_by_job_id.get(job_id, set()) + def generate_artifact_url( self, *, diff --git a/apps/api/tests/contract/test_documents_contract.py b/apps/api/tests/contract/test_documents_contract.py index 6b8e33b04..ebe27e64f 100644 --- a/apps/api/tests/contract/test_documents_contract.py +++ b/apps/api/tests/contract/test_documents_contract.py @@ -533,6 +533,22 @@ def _upload_page_citation_asset(*, job_id: str, artifact_ref: str) -> None: asset_path.unlink(missing_ok=True) +def _upload_chunk_asset(*, job_id: str, artifact_ref: str) -> None: + from shared.services.storage.result_storage import JobResultStorage + + suffix = Path(artifact_ref).suffix or ".bin" + asset_path = Path("/tmp") / f"knowhere-contract-chunk-asset-{uuid4().hex}{suffix}" + asset_path.write_bytes(b"
contract chunk asset
") + try: + JobResultStorage().upload_raw_file( + job_id=job_id, + relative_path=artifact_ref, + local_file_path=str(asset_path), + ) + finally: + asset_path.unlink(missing_ok=True) + + @pytest.mark.asyncio async def test_should_list_only_the_authenticated_users_documents_for_the_effective_namespace( developer_api_client_factory: Callable[ @@ -992,6 +1008,10 @@ async def test_should_include_media_asset_urls_in_document_chunk_list_when_reque }, ], ) + _upload_chunk_asset( + job_id=revision["job_id"], + artifact_ref="tables/table-1.html", + ) response = await api_client.get( f"/api/v1/documents/{document_id}/chunks", params={ @@ -1167,6 +1187,10 @@ async def test_should_return_one_document_chunk_by_document_chunk_id( } ], ) + _upload_chunk_asset( + job_id=revision["job_id"], + artifact_ref="images/figure-1.png", + ) response = await api_client.get( f"/api/v1/documents/{document_id}/chunks/{chunk_id}", params={"include_asset_urls": "true"}, diff --git a/apps/api/tests/contract/test_retrieval_contract.py b/apps/api/tests/contract/test_retrieval_contract.py index 58de969c7..4debea613 100644 --- a/apps/api/tests/contract/test_retrieval_contract.py +++ b/apps/api/tests/contract/test_retrieval_contract.py @@ -555,6 +555,9 @@ def normalize_artifact_ref(self, artifact_ref: str | None) -> str | None: return None return normalized + def verify_raw_exists(self, *, job_id: str, relative_path: str) -> bool: + return True + def fake_get_result_storage() -> FakeResultStorage: return FakeResultStorage() diff --git a/apps/api/tests/unit/test_document_chunk_asset_url.py b/apps/api/tests/unit/test_document_chunk_asset_url.py new file mode 100644 index 000000000..84d2739b6 --- /dev/null +++ b/apps/api/tests/unit/test_document_chunk_asset_url.py @@ -0,0 +1,177 @@ +"""Unit tests for document chunk asset URL generation.""" + +from __future__ import annotations + +import sys +from pathlib import Path +from types import ModuleType +from typing import Any +from unittest.mock import patch + +import pytest +from tests.support.import_environment import ( + configure_import_environment, + ensure_import_paths, +) + +# Defer importing apps/api `app` until test bodies run. Module-level imports +# would cache API's `app` package and break apps/worker contract collection in +# the same pytest process (both packages are named `app`). +configure_import_environment() +ensure_import_paths() + +_API_ROOT = str(Path(__file__).resolve().parents[2]) + + +def _is_api_app_module(module: ModuleType | None) -> bool: + if module is None: + return False + module_file = getattr(module, "__file__", None) + if isinstance(module_file, str) and module_file.startswith(_API_ROOT): + return True + module_paths = getattr(module, "__path__", ()) + try: + return any(str(path).startswith(_API_ROOT) for path in module_paths) + except KeyError: + return False + + +def _drop_api_app_modules() -> None: + for module_name in sorted(sys.modules, key=len, reverse=True): + if module_name != "app" and not module_name.startswith("app."): + continue + if _is_api_app_module(sys.modules.get(module_name)): + sys.modules.pop(module_name, None) + + +def _prioritize_api_import_root() -> None: + """Keep apps/api ahead of apps/worker for the shared `app` package name.""" + ensure_import_paths() + if _API_ROOT in sys.path: + sys.path.remove(_API_ROOT) + sys.path.insert(0, _API_ROOT) + + +@pytest.fixture(autouse=True) +def _clear_api_app_modules_after_unit_test(): + """Avoid leaving API's `app` package cached for later worker contract tests.""" + yield + _drop_api_app_modules() + + +class FakeResultStorage: + def __init__(self, *, exists: bool = True) -> None: + self.exists = exists + self.checked: list[str] = [] + + def verify_raw_exists(self, *, job_id: str, relative_path: str) -> bool: + self.checked.append(f"{job_id}/{relative_path}") + return self.exists + + def generate_artifact_url( + self, + *, + job_id: str, + artifact_ref: str, + expires_in: int = 3600, + ) -> str | None: + return f"https://assets.example.test/{job_id}/{artifact_ref}" + + +def _document_chunk_asset_url(**kwargs: Any) -> str | None: + _prioritize_api_import_root() + _drop_api_app_modules() + from app.services.documents.lifecycle_service import ( + _document_chunk_asset_url, + ) + + return _document_chunk_asset_url(**kwargs) + + +def test_asset_url_generated_when_raw_file_exists() -> None: + storage = FakeResultStorage(exists=True) + + url = _document_chunk_asset_url( + chunk_type="table", + job_id="job-1", + file_path="tables/table-1 Test.html", + include_asset_urls=True, + result_storage=storage, + ) + + assert url == "https://assets.example.test/job-1/tables/table-1 Test.html" + assert storage.checked == ["job-1/tables/table-1 Test.html"] + + +def test_asset_url_skipped_when_raw_file_missing() -> None: + storage = FakeResultStorage(exists=False) + + url = _document_chunk_asset_url( + chunk_type="table", + job_id="job-1", + file_path="tables/table-1 Test.html", + include_asset_urls=True, + result_storage=storage, + ) + + assert url is None + assert storage.checked == ["job-1/tables/table-1 Test.html"] + + +def test_asset_url_falls_back_on_verification_error() -> None: + class ExplodingStorage: + def verify_raw_exists(self, *, job_id: str, relative_path: str) -> bool: + raise RuntimeError("storage unavailable") + + def generate_artifact_url( + self, + *, + job_id: str, + artifact_ref: str, + expires_in: int = 3600, + ) -> str | None: + return f"https://assets.example.test/{job_id}/{artifact_ref}" + + with patch( + "app.services.documents.lifecycle_service.logger.warning" + ) as mock_warning: + url = _document_chunk_asset_url( + chunk_type="image", + job_id="job-1", + file_path="images/page-1.png", + include_asset_urls=True, + result_storage=ExplodingStorage(), + ) + + assert url is None + assert mock_warning.call_count == 1 + + +def test_asset_url_skipped_for_non_media_chunk_types() -> None: + storage = FakeResultStorage(exists=True) + + url = _document_chunk_asset_url( + chunk_type="text", + job_id="job-1", + file_path="tables/table-1 Test.html", + include_asset_urls=True, + result_storage=storage, + ) + + assert url is None + assert storage.checked == [] + + +def test_asset_url_skipped_when_include_asset_urls_false() -> None: + storage = FakeResultStorage(exists=True) + + url = _document_chunk_asset_url( + chunk_type="table", + job_id="job-1", + file_path="tables/table-1 Test.html", + include_asset_urls=False, + result_storage=storage, + ) + + assert url is None + assert storage.checked == []