diff --git a/pyrit/backend/services/converter_service.py b/pyrit/backend/services/converter_service.py index e85f91be47..54d463bb75 100644 --- a/pyrit/backend/services/converter_service.py +++ b/pyrit/backend/services/converter_service.py @@ -173,8 +173,9 @@ async def create_converter_async(self, *, request: CreateConverterRequest) -> Co """ Create a new converter instance from API request. - Instantiates the converter with the given type and params, - then registers it in the registry. + Instantiates the converter with the given type and params and builds the + response before registering it, so a request that fails at any step leaves + no registered converter and removes its uploaded files. Args: request: The create converter request with type and params. @@ -194,20 +195,20 @@ async def create_converter_async(self, *, request: CreateConverterRequest) -> Co params=request.params, ) try: - converter_obj = self._registry.create_named_instance( + # Uploads may have yielded to another request that took the name. + self._registry.instances.validate_name_available(request.name) + converter_obj = self._registry.create_instance(request.type, **params) + converter = self._build_instance_from_object(converter_id=request.name, converter_obj=converter_obj) + self._registry.instances.register( + converter_obj, name=request.name, - type_name=request.type, - params=params, - registry_metadata={_OWNED_ARTIFACT_PATHS_KEY: [str(path) for path in owned_paths]}, + metadata={_OWNED_ARTIFACT_PATHS_KEY: [str(path) for path in owned_paths]}, ) except (Exception, asyncio.CancelledError): await self._remove_owned_artifacts_async(paths=owned_paths) raise - return self._build_instance_from_object( - converter_id=request.name, - converter_obj=converter_obj, - ) + return converter async def preview_conversion_async(self, *, request: ConverterPreviewRequest) -> ConverterPreviewResponse: """ diff --git a/pyrit/backend/services/target_service.py b/pyrit/backend/services/target_service.py index d57188f2c7..192bda7253 100644 --- a/pyrit/backend/services/target_service.py +++ b/pyrit/backend/services/target_service.py @@ -183,7 +183,8 @@ async def create_target_async(self, *, request: CreateTargetRequest) -> TargetIn by the target classes themselves. This service only enforces the request-level auth contract: for ``identity`` it confirms the target supports it and omits the api_key so the target validates its own - endpoint and authenticates itself. + endpoint and authenticates itself. The response is built before the + target is registered, so a failed request leaves no registered target. Args: request: The create target request with type, params, and auth_mode. @@ -214,12 +215,11 @@ async def create_target_async(self, *, request: CreateTargetRequest) -> TargetIn # LEGACY COMPATIBILITY: The current configuration UI omits the name. # Remove this generated fallback after that UI sends an explicit name. target_registry_name = request.name or f"compat_{uuid.uuid4().hex}" - target_obj = self._registry.create_named_instance( - name=target_registry_name, - type_name=request.type, - params=params, - ) - return self._build_instance_from_object(target_registry_name=target_registry_name, target_obj=target_obj) + self._registry.instances.validate_name_available(target_registry_name) + target_obj = self._registry.create_instance(request.type, **params) + target = self._build_instance_from_object(target_registry_name=target_registry_name, target_obj=target_obj) + self._registry.instances.register(target_obj, name=target_registry_name) + return target @lru_cache(maxsize=1) diff --git a/tests/unit/backend/test_converter_service.py b/tests/unit/backend/test_converter_service.py index 26497ac54b..036c111bc1 100644 --- a/tests/unit/backend/test_converter_service.py +++ b/tests/unit/backend/test_converter_service.py @@ -783,6 +783,69 @@ async def test_create_converter_cleans_upload_when_construction_fails( assert service._registry.instances.get("invalid-pdf") is None assert list(service._upload_path.iterdir()) == [] + async def test_create_converter_registers_nothing_when_response_mapping_fails( + self, upload_service: ConverterService + ) -> None: + request = CreateConverterRequest( + name="unmapped-pdf", + type="PDFConverter", + params={"existing_pdf": _make_data_uri(mime_type="application/pdf", content=b"%PDF-1.4\n")}, + ) + + with ( + patch( + "pyrit.backend.services.converter_service.converter_object_to_instance", + side_effect=RuntimeError("mapping failed"), + ), + pytest.raises(RuntimeError, match="mapping failed"), + ): + await upload_service.create_converter_async(request=request) + + assert upload_service._registry.instances.get("unmapped-pdf") is None + assert list(upload_service._upload_path.iterdir()) == [] + + async def test_create_converter_rechecks_name_after_upload(self, upload_service: ConverterService) -> None: + persist_async = upload_service._persist_data_uri_params_async + + async def persist_then_take_name_async(**kwargs: object) -> object: + result = await persist_async(**kwargs) + upload_service._registry.instances.register(Base64Converter(), name="taken") + return result + + request = CreateConverterRequest( + name="taken", + type="PDFConverter", + params={"existing_pdf": _make_data_uri(mime_type="application/pdf", content=b"%PDF-1.4\n")}, + ) + with ( + patch.object(upload_service, "_persist_data_uri_params_async", side_effect=persist_then_take_name_async), + patch.object(upload_service._registry, "create_instance") as create_instance, + pytest.raises(ValueError, match="already exists"), + ): + await upload_service.create_converter_async(request=request) + + create_instance.assert_not_called() + assert list(upload_service._upload_path.iterdir()) == [] + + async def test_create_converter_removes_upload_when_registration_fails( + self, upload_service: ConverterService + ) -> None: + request = CreateConverterRequest( + name="raced-pdf", + type="PDFConverter", + params={"existing_pdf": _make_data_uri(mime_type="application/pdf", content=b"%PDF-1.4\n")}, + ) + + with ( + patch.object( + upload_service._registry.instances, "register", side_effect=ValueError("'raced-pdf' already exists") + ), + pytest.raises(ValueError, match="already exists"), + ): + await upload_service.create_converter_async(request=request) + + assert list(upload_service._upload_path.iterdir()) == [] + @pytest.mark.parametrize("error", [OSError("write failed"), asyncio.CancelledError()]) async def test_persist_data_uri_cleans_partial_write( self, upload_service: ConverterService, error: BaseException diff --git a/tests/unit/backend/test_target_service.py b/tests/unit/backend/test_target_service.py index ac997198b2..ccf2e7d2c4 100644 --- a/tests/unit/backend/test_target_service.py +++ b/tests/unit/backend/test_target_service.py @@ -415,6 +415,22 @@ async def test_create_target_rejects_reserved_route_name(self, sqlite_instance, request=CreateTargetRequest(name=name, type="TextTarget", params={}), ) + async def test_create_target_registers_nothing_when_response_mapping_fails(self, sqlite_instance) -> None: + service = TargetService() + + with ( + patch( + "pyrit.backend.services.target_service.target_object_to_instance", + side_effect=RuntimeError("mapping failed"), + ), + pytest.raises(RuntimeError, match="mapping failed"), + ): + await service.create_target_async( + request=CreateTargetRequest(name="unmapped", type="TextTarget", params={}), + ) + + assert service.get_target_object(target_registry_name="unmapped") is None + async def test_create_target_delegates_construction_to_registry(self, sqlite_instance) -> None: """Every target construction path is owned by the registry.""" service = TargetService()