fix: add logging to empty exception handlers in client destructors#3444
fix: add logging to empty exception handlers in client destructors#3444C1-BA-B1-F3 wants to merge 5 commits into
Conversation
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 openai#3325
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 6132922 which removed the drain. Fixes openai#3440
…nnotation 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 openai#3419 Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
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 openai#3428
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b89b1e38e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for _ in iterator: | ||
| pass | ||
| response.close() |
There was a problem hiding this comment.
If the cleanup drain raises while reading the remainder of the stream (for example a ReadTimeout/ReadError from a proxy or server before EOF), execution exits this finally block before response.close() runs. That regresses the previous guarantee that the response is released, and can also turn a stream that already delivered [DONE] into a cleanup-time read failure; wrap the drain so close still runs in a nested finally (the async drain below has the same pattern).
Useful? React with 👍 / 👎.
| """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"] |
There was a problem hiding this comment.
This test uses the Pydantic v2-only model_fields attribute, but the repo still supports and runs a test-pydantic-v1 nox session that installs pydantic<2; under that job ResponseOutputTextAnnotationAddedEvent only has __fields__, so this new test fails before checking the annotation type. Use openai._compat.get_model_fields(...) as the existing tests do for cross-version field inspection.
Useful? React with 👍 / 👎.
- 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
1 participant
Summary
This PR addresses issue #3428 by replacing the empty
except: passexception handlers inSyncHttpxClientWrapper.__del__andAsyncHttpxClientWrapper.__del__with proper debug logging.Changes
src/openai/_base_client.py:SyncHttpxClientWrapper.__del__: Changedexcept Exception: passtoexcept Exception: log.debug("Failed to close client in destructor", exc_info=True)AsyncHttpxClientWrapper.__del__: Changedexcept Exception: passtoexcept Exception: log.debug("Failed to close async client in destructor", exc_info=True)tests/test_destructor_logging.py(new):close()/aclose()failsWhy This Approach
DEBUGlevel because destructor failures are typically not critical (the client is being garbage collected)exc_info=True: Preserves the full traceback for debugging purposes__del__loglogger already defined at module levelFixes #3428