From d91d0d75c2846f56c73fcec66923ea5f26974991 Mon Sep 17 00:00:00 2001 From: C1-BA-B1-F3 Date: Fri, 26 Jun 2026 03:15:19 +0800 Subject: [PATCH 1/5] fix: handle null response.output in parse_response When a backend sends 'response.output: null' in a response.completed event (e.g., chatgpt.com Codex backend), parse_response would crash with TypeError: 'NoneType' object is not iterable. This one-line fix coerces None to an empty list, allowing the stream to complete gracefully. Consumers that track output_item.done events can still backfill from their collected items. Fixes #3325 --- src/openai/lib/_parsing/_responses.py | 2 +- tests/lib/test_parsing_responses.py | 87 +++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 tests/lib/test_parsing_responses.py 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/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 From 7dc030f8de39bf1b874ebaeb22f85d4ca59294e8 Mon Sep 17 00:00:00 2001 From: C1-BA-B1-F3 Date: Fri, 26 Jun 2026 03:29:38 +0800 Subject: [PATCH 2/5] fix: drain remaining SSE events after [DONE] to prevent TCP FIN After receiving the [DONE] SSE event, Stream.__stream__ and AsyncStream.__stream__ now drain remaining events from the iterator before calling response.close(). This ensures the HTTP/1.1 chunked transfer-encoding terminator (0\r\n\r\n) is consumed, allowing h11's their_state to advance to DONE so httpcore returns the connection to the pool gracefully instead of destroying it with TCP FIN. Regression from 6132922c which removed the drain. Fixes #3440 --- src/openai/_streaming.py | 14 ++++++++++++-- tests/test_streaming.py | 27 +++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/openai/_streaming.py b/src/openai/_streaming.py index 45c13cc11d..2251792ae7 100644 --- a/src/openai/_streaming.py +++ b/src/openai/_streaming.py @@ -106,7 +106,14 @@ def __stream__(self) -> Iterator[_T]: response=response, ) finally: - # Ensure the response is closed even if the consumer doesn't read all data + # 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 + for _ in iterator: + pass response.close() def __enter__(self) -> Self: @@ -216,7 +223,10 @@ async def __stream__(self) -> AsyncIterator[_T]: response=response, ) finally: - # Ensure the response is closed even if the consumer doesn't read all data + # Drain remaining events so the chunked terminator is consumed before close. + # See: https://github.com/openai/openai-python/issues/3440 + async for _ in iterator: + pass await response.aclose() async def __aenter__(self) -> Self: 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 From 4e0b2a1df117fc765a2646cc7f1e0785c0235393 Mon Sep 17 00:00:00 2001 From: C1-BA-B1-F3 Date: Fri, 26 Jun 2026 04:57:05 +0800 Subject: [PATCH 3/5] fix: use Annotation type for ResponseOutputTextAnnotationAddedEvent.annotation The annotation field was typed as 'object' instead of the proper 'Annotation' union type (AnnotationFileCitation | AnnotationURLCitation | AnnotationContainerFileCitation | AnnotationFilePath). This forced consumers to use typing.cast to access typed fields. Fixes #3419 Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- ...onse_output_text_annotation_added_event.py | 3 +- tests/test_annotation_event.py | 143 ++++++++++++++++++ 2 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 tests/test_annotation_event.py 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/test_annotation_event.py b/tests/test_annotation_event.py new file mode 100644 index 0000000000..a2084f9d93 --- /dev/null +++ b/tests/test_annotation_event.py @@ -0,0 +1,143 @@ +# Tests for https://github.com/openai/openai-python/issues/3419 +# ResponseOutputTextAnnotationAddedEvent.annotation should be typed as Annotation, not object + +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 = ResponseOutputTextAnnotationAddedEvent.model_fields["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" From 5b89b1e38e0a46ccc7f949a548fc36b082d52d9a Mon Sep 17 00:00:00 2001 From: C1-BA-B1-F3 Date: Fri, 26 Jun 2026 04:59:49 +0800 Subject: [PATCH 4/5] fix: add logging to empty exception handlers in client destructors Previously, the __del__ methods of SyncHttpxClientWrapper and AsyncHttpxClientWrapper silently swallowed all exceptions with bare 'except: pass' patterns, making debugging difficult. Now these handlers log the exception at DEBUG level using the existing logger, preserving the original behavior of not raising in destructors while providing visibility into failures. Fixes #3428 --- src/openai/_base_client.py | 4 +- tests/test_destructor_logging.py | 99 ++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 2 deletions(-) create mode 100644 tests/test_destructor_logging.py 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/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() From 5678e0c273e71352eadbf833dcf423289b57d245 Mon Sep 17 00:00:00 2001 From: C1-BA-B1-F3 Date: Fri, 26 Jun 2026 06:13:45 +0800 Subject: [PATCH 5/5] fix: address PR review feedback - Wrap stream drain in nested finally so response.close() always runs even if drain raises (sync + async) - Use openai._compat.get_model_fields() instead of model_fields for Pydantic v1 compatibility in tests --- src/openai/_streaming.py | 16 ++++++++++------ tests/test_annotation_event.py | 3 ++- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/openai/_streaming.py b/src/openai/_streaming.py index 2251792ae7..84ee284c29 100644 --- a/src/openai/_streaming.py +++ b/src/openai/_streaming.py @@ -112,9 +112,11 @@ def __stream__(self) -> Iterator[_T]: # 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 - for _ in iterator: - pass - response.close() + try: + for _ in iterator: + pass + finally: + response.close() def __enter__(self) -> Self: return self @@ -225,9 +227,11 @@ async def __stream__(self) -> AsyncIterator[_T]: finally: # Drain remaining events so the chunked terminator is consumed before close. # See: https://github.com/openai/openai-python/issues/3440 - async for _ in iterator: - pass - await response.aclose() + try: + async for _ in iterator: + pass + finally: + await response.aclose() async def __aenter__(self) -> Self: return self diff --git a/tests/test_annotation_event.py b/tests/test_annotation_event.py index a2084f9d93..a85687d957 100644 --- a/tests/test_annotation_event.py +++ b/tests/test_annotation_event.py @@ -1,6 +1,7 @@ # 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 ( @@ -117,7 +118,7 @@ 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 = ResponseOutputTextAnnotationAddedEvent.model_fields["annotation"] + field_info = get_model_fields(ResponseOutputTextAnnotationAddedEvent)["annotation"] annotation_type = field_info.annotation # The Annotation type is an Annotated[Union[...], ...]