diff --git a/src/openai/_base_client.py b/src/openai/_base_client.py index 216b36aabd..cb0ef7ca6d 100644 --- a/src/openai/_base_client.py +++ b/src/openai/_base_client.py @@ -859,7 +859,7 @@ def __del__(self) -> None: try: self.close() except Exception: - pass + log.debug("Failed to close client in destructor", exc_info=True) class SyncAPIClient(BaseClient[httpx.Client, Stream[Any]]): @@ -1469,7 +1469,7 @@ def __del__(self) -> None: # TODO(someday): support non asyncio runtimes here asyncio.get_running_loop().create_task(self.aclose()) except Exception: - pass + log.debug("Failed to close async client in destructor", exc_info=True) class AsyncAPIClient(BaseClient[httpx.AsyncClient, AsyncStream[Any]]): diff --git a/src/openai/_streaming.py b/src/openai/_streaming.py index 45c13cc11d..84ee284c29 100644 --- a/src/openai/_streaming.py +++ b/src/openai/_streaming.py @@ -106,8 +106,17 @@ def __stream__(self) -> Iterator[_T]: response=response, ) finally: - # Ensure the response is closed even if the consumer doesn't read all data - response.close() + # Drain remaining events from the iterator so that the underlying + # response.iter_bytes() is fully consumed, including the HTTP/1.1 + # chunked transfer-encoding terminator (0\r\n\r\n). Without this, + # h11's their_state won't advance to DONE, causing httpcore to + # destroy the connection (TCP FIN) instead of returning it to the pool. + # See: https://github.com/openai/openai-python/issues/3440 + try: + for _ in iterator: + pass + finally: + response.close() def __enter__(self) -> Self: return self @@ -216,8 +225,13 @@ async def __stream__(self) -> AsyncIterator[_T]: response=response, ) finally: - # Ensure the response is closed even if the consumer doesn't read all data - await response.aclose() + # Drain remaining events so the chunked terminator is consumed before close. + # See: https://github.com/openai/openai-python/issues/3440 + try: + async for _ in iterator: + pass + finally: + await response.aclose() async def __aenter__(self) -> Self: return self diff --git a/src/openai/lib/_parsing/_responses.py b/src/openai/lib/_parsing/_responses.py index 232718cef6..ad2297e249 100644 --- a/src/openai/lib/_parsing/_responses.py +++ b/src/openai/lib/_parsing/_responses.py @@ -58,7 +58,7 @@ def parse_response( ) -> ParsedResponse[TextFormatT]: output_list: List[ParsedResponseOutputItem[TextFormatT]] = [] - for output in response.output: + for output in (response.output or []): if output.type == "message": content_list: List[ParsedContent[TextFormatT]] = [] for item in output.content: diff --git a/src/openai/types/responses/response_output_text_annotation_added_event.py b/src/openai/types/responses/response_output_text_annotation_added_event.py index b9dc262150..a2f256cb97 100644 --- a/src/openai/types/responses/response_output_text_annotation_added_event.py +++ b/src/openai/types/responses/response_output_text_annotation_added_event.py @@ -3,6 +3,7 @@ from typing_extensions import Literal from ..._models import BaseModel +from .response_output_text import Annotation __all__ = ["ResponseOutputTextAnnotationAddedEvent"] @@ -10,7 +11,7 @@ class ResponseOutputTextAnnotationAddedEvent(BaseModel): """Emitted when an annotation is added to output text content.""" - annotation: object + annotation: Annotation """The annotation object being added. (See annotation schema for details.)""" annotation_index: int diff --git a/tests/lib/test_parsing_responses.py b/tests/lib/test_parsing_responses.py new file mode 100644 index 0000000000..aab8d66567 --- /dev/null +++ b/tests/lib/test_parsing_responses.py @@ -0,0 +1,87 @@ +"""Tests for parse_response handling of null/None output fields.""" + +from __future__ import annotations + +from openai._models import construct_type_unchecked +from openai._types import Omit +from openai.lib._parsing._responses import parse_response +from openai.types.responses import Response, ParsedResponse + + +def _make_response(output=None, **kwargs): + """Helper to construct a Response with a given output field.""" + base = { + "id": "resp_test123", + "created_at": 1234567890.0, + "model": "gpt-4o", + "object": "response", + "status": "completed", + "output": output, + "parallel_tool_calls": True, + "tool_choice": "auto", + "tools": [], + "temperature": 1.0, + "top_p": 1.0, + } + base.update(kwargs) + return construct_type_unchecked(type_=Response, value=base) + + +def test_parse_response_with_none_output(): + """Test that parse_response handles null output without crashing.""" + response = _make_response(output=None) + assert response.output is None + + result = parse_response( + text_format=None, + input_tools=None, + response=response, + ) + + assert isinstance(result, ParsedResponse) + assert result.output == [] + + +def test_parse_response_with_empty_list_output(): + """Test that parse_response handles empty list output correctly.""" + response = _make_response(output=[]) + assert response.output == [] + + result = parse_response( + text_format=None, + input_tools=None, + response=response, + ) + + assert isinstance(result, ParsedResponse) + assert result.output == [] + + +def test_parse_response_with_message_output(): + """Test that parse_response still works correctly with actual output items.""" + output_data = [ + { + "id": "msg_test123", + "type": "message", + "status": "completed", + "role": "assistant", + "content": [ + { + "type": "output_text", + "text": "Hello, world!", + "annotations": [], + } + ], + } + ] + response = _make_response(output=output_data) + + result = parse_response( + text_format=Omit(), + input_tools=None, + response=response, + ) + + assert isinstance(result, ParsedResponse) + assert len(result.output) == 1 + assert result.output[0].type == "message" \ No newline at end of file diff --git a/tests/test_annotation_event.py b/tests/test_annotation_event.py new file mode 100644 index 0000000000..a85687d957 --- /dev/null +++ b/tests/test_annotation_event.py @@ -0,0 +1,144 @@ +# Tests for https://github.com/openai/openai-python/issues/3419 +# ResponseOutputTextAnnotationAddedEvent.annotation should be typed as Annotation, not object + +from openai._compat import get_model_fields +from openai._models import construct_type +from openai.types.responses.response_output_text_annotation_added_event import ResponseOutputTextAnnotationAddedEvent +from openai.types.responses.response_output_text import ( + Annotation, + AnnotationFileCitation, + AnnotationURLCitation, + AnnotationContainerFileCitation, + AnnotationFilePath, +) + + +def test_annotation_url_citation() -> None: + """Test that url_citation annotation is properly typed and parsed.""" + event = ResponseOutputTextAnnotationAddedEvent( + annotation={ + "type": "url_citation", + "title": "Example", + "url": "https://example.com", + "start_index": 0, + "end_index": 10, + }, + annotation_index=0, + content_index=0, + item_id="item_123", + output_index=0, + sequence_number=1, + type="response.output_text.annotation.added", + ) + + # After the fix, annotation should be an AnnotationURLCitation, not plain object + assert isinstance(event.annotation, AnnotationURLCitation) + assert event.annotation.type == "url_citation" + assert event.annotation.title == "Example" + assert event.annotation.url == "https://example.com" + assert event.annotation.start_index == 0 + assert event.annotation.end_index == 10 + + +def test_annotation_file_citation() -> None: + """Test that file_citation annotation is properly typed and parsed.""" + event = ResponseOutputTextAnnotationAddedEvent( + annotation={ + "type": "file_citation", + "file_id": "file_abc123", + "filename": "document.pdf", + "index": 0, + }, + annotation_index=0, + content_index=0, + item_id="item_123", + output_index=0, + sequence_number=1, + type="response.output_text.annotation.added", + ) + + assert isinstance(event.annotation, AnnotationFileCitation) + assert event.annotation.type == "file_citation" + assert event.annotation.file_id == "file_abc123" + assert event.annotation.filename == "document.pdf" + assert event.annotation.index == 0 + + +def test_annotation_container_file_citation() -> None: + """Test that container_file_citation annotation is properly typed and parsed.""" + event = ResponseOutputTextAnnotationAddedEvent( + annotation={ + "type": "container_file_citation", + "container_id": "container_abc", + "file_id": "file_abc123", + "filename": "document.pdf", + "start_index": 0, + "end_index": 10, + }, + annotation_index=0, + content_index=0, + item_id="item_123", + output_index=0, + sequence_number=1, + type="response.output_text.annotation.added", + ) + + assert isinstance(event.annotation, AnnotationContainerFileCitation) + assert event.annotation.type == "container_file_citation" + assert event.annotation.container_id == "container_abc" + assert event.annotation.file_id == "file_abc123" + assert event.annotation.filename == "document.pdf" + assert event.annotation.start_index == 0 + assert event.annotation.end_index == 10 + + +def test_annotation_file_path() -> None: + """Test that file_path annotation is properly typed and parsed.""" + event = ResponseOutputTextAnnotationAddedEvent( + annotation={ + "type": "file_path", + "file_id": "file_abc123", + "index": 0, + }, + annotation_index=0, + content_index=0, + item_id="item_123", + output_index=0, + sequence_number=1, + type="response.output_text.annotation.added", + ) + + assert isinstance(event.annotation, AnnotationFilePath) + assert event.annotation.type == "file_path" + assert event.annotation.file_id == "file_abc123" + assert event.annotation.index == 0 + + +def test_annotation_type_is_union() -> None: + """Test that the annotation field accepts all annotation types.""" + # Verify the field is typed as Annotation (Union of all annotation types), not object + # We check by inspecting the model's field info directly + field_info = get_model_fields(ResponseOutputTextAnnotationAddedEvent)["annotation"] + annotation_type = field_info.annotation + + # The Annotation type is an Annotated[Union[...], ...] + # We can verify it's not just 'object' + assert annotation_type is not object + + +def test_construct_type_with_annotation() -> None: + """Test that construct_type works with the Annotation type for streaming events.""" + # This simulates how streaming events are parsed + annotation_data = { + "type": "url_citation", + "title": "Test", + "url": "https://test.com", + "start_index": 5, + "end_index": 15, + } + + # construct_type should be able to parse this as an Annotation + annotation = construct_type(value=annotation_data, type_=Annotation) + assert isinstance(annotation, AnnotationURLCitation) + assert annotation.title == "Test" + assert annotation.url == "https://test.com" diff --git a/tests/test_destructor_logging.py b/tests/test_destructor_logging.py new file mode 100644 index 0000000000..1918bdb5d9 --- /dev/null +++ b/tests/test_destructor_logging.py @@ -0,0 +1,99 @@ +"""Tests to verify that destructor exception handlers log errors instead of silently swallowing them.""" + +from __future__ import annotations + +import gc +import logging +from unittest import mock + +import httpx +import pytest + +from openai._base_client import SyncHttpxClientWrapper, AsyncHttpxClientWrapper + + +class TestSyncDestructorLogging: + """Test that SyncHttpxClientWrapper.__del__ logs exceptions instead of silently swallowing them.""" + + def test_del_logs_on_close_error(self, caplog: pytest.LogCaptureFixture) -> None: + """Test that __del__ logs when close() raises an exception.""" + client = SyncHttpxClientWrapper() + + # Mock close() to raise an exception + with mock.patch.object(client, "close", side_effect=RuntimeError("close failed")): + with caplog.at_level(logging.DEBUG, logger="openai._base_client"): + client.__del__() + + assert any( + "Failed to close client in destructor" in record.message + for record in caplog.records + ), f"Expected log message not found in: {[r.message for r in caplog.records]}" + + def test_del_no_log_on_success(self, caplog: pytest.LogCaptureFixture) -> None: + """Test that __del__ doesn't log when close() succeeds.""" + client = SyncHttpxClientWrapper() + + with caplog.at_level(logging.DEBUG, logger="openai._base_client"): + client.__del__() + + assert not any( + "Failed to close client in destructor" in record.message + for record in caplog.records + ) + + def test_del_skips_if_closed(self, caplog: pytest.LogCaptureFixture) -> None: + """Test that __del__ skips close() if already closed.""" + client = SyncHttpxClientWrapper() + client.close() + + with mock.patch.object(client, "close") as mock_close: + with caplog.at_level(logging.DEBUG, logger="openai._base_client"): + client.__del__() + + mock_close.assert_not_called() + + +class TestAsyncDestructorLogging: + """Test that AsyncHttpxClientWrapper.__del__ logs exceptions instead of silently swallowing them.""" + + def test_del_logs_when_no_event_loop(self, caplog: pytest.LogCaptureFixture) -> None: + """Test that __del__ logs when asyncio.get_running_loop() raises (no event loop).""" + client = AsyncHttpxClientWrapper() + + with mock.patch("asyncio.get_running_loop", side_effect=RuntimeError("no running event loop")): + with caplog.at_level(logging.DEBUG, logger="openai._base_client"): + client.__del__() + # Mark as closed so GC won't trigger __del__ again + client._state = httpx._client.ClientState.CLOSED + + assert any( + "Failed to close async client in destructor" in record.message + for record in caplog.records + ), f"Expected log message not found in: {[r.message for r in caplog.records]}" + + def test_del_no_log_on_success(self, caplog: pytest.LogCaptureFixture) -> None: + """Test that __del__ doesn't log when it successfully creates a close task.""" + client = AsyncHttpxClientWrapper() + + mock_loop = mock.MagicMock() + with mock.patch("asyncio.get_running_loop", return_value=mock_loop): + with caplog.at_level(logging.DEBUG, logger="openai._base_client"): + client.__del__() + # Mark as closed so GC won't trigger __del__ again + client._state = httpx._client.ClientState.CLOSED + + assert not any( + "Failed to close async client in destructor" in record.message + for record in caplog.records + ), f"Unexpected log messages: {[r.message for r in caplog.records]}" + + def test_del_skips_if_closed(self, caplog: pytest.LogCaptureFixture) -> None: + """Test that __del__ skips if already closed.""" + client = AsyncHttpxClientWrapper() + client._state = httpx._client.ClientState.CLOSED + + with mock.patch("asyncio.get_running_loop") as mock_loop: + with caplog.at_level(logging.DEBUG, logger="openai._base_client"): + client.__del__() + + mock_loop.assert_not_called() diff --git a/tests/test_streaming.py b/tests/test_streaming.py index 04f8e51abd..c4f3399627 100644 --- a/tests/test_streaming.py +++ b/tests/test_streaming.py @@ -246,3 +246,30 @@ def make_event_iterator( return AsyncStream( cast_to=object, client=async_client, response=httpx.Response(200, content=to_aiter(content)) )._iter_events() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("sync", [True, False], ids=["sync", "async"]) +async def test_stream_drains_before_close(sync: bool, client: OpenAI, async_client: AsyncOpenAI) -> None: + """Regression test for https://github.com/openai/openai-python/issues/3440 + + After [DONE], the response stream should be fully drained (including the + chunked transfer-encoding terminator) before close() is called. + """ + chunks = [ + b'data: {"choices":[{"delta":{"content":"hi"}}]}\n\n', + b'data: [DONE]\n\n', + ] + + if sync: + response = httpx.Response(200, content=iter(chunks)) + stream = Stream(cast_to=object, client=client, response=response) + for _ in stream: + pass + assert response.is_stream_consumed + else: + response = httpx.Response(200, content=to_aiter(iter(chunks))) + stream = AsyncStream(cast_to=object, client=async_client, response=response) + async for _ in stream: + pass + assert response.is_stream_consumed