Skip to content

Close HTTP responses when error handling throws - #746

Closed
fallintoplace wants to merge 1 commit into
openai:mainfrom
fallintoplace:fix/close-error-responses
Closed

fallintoplace wants to merge 1 commit into
openai:mainfrom
fallintoplace:fix/close-error-responses

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • close non-2xx HttpResponse instances inside errorHandler before throwing typed service exceptions
  • keep 2xx responses open and returned as before
  • copy status, headers, and parsed error data before the response is closed
  • add handler-level tests covering success, typed errors, unexpected statuses, and error body handler failures

Fixes #745.

Test plan

  • JAVA_HOME=/opt/homebrew/opt/openjdk@21/libexec/openjdk.jdk/Contents/Home ./gradlew :openai-java-core:format
  • JAVA_HOME=/opt/homebrew/opt/openjdk@21/libexec/openjdk.jdk/Contents/Home ./gradlew :openai-java-core:test --tests com.openai.core.handlers.ErrorHandlerTest
  • JAVA_HOME=/opt/homebrew/opt/openjdk@21/libexec/openjdk.jdk/Contents/Home ./gradlew :openai-java-core:lint
  • JAVA_HOME=/opt/homebrew/opt/openjdk@21/libexec/openjdk.jdk/Contents/Home ./gradlew :openai-java-core:test --tests com.openai.services.ErrorHandlingTest

@fallintoplace
fallintoplace marked this pull request as ready for review May 19, 2026 18:17
@fallintoplace
fallintoplace requested a review from a team as a code owner May 19, 2026 18:17
@dpiet-oai

Copy link
Copy Markdown
Contributor

Thank you for the fix and the thoughtful tests! We combined your work with #879 in #1105, which is now merged, and credited both contributors. Closing this PR as superseded. We’re keeping #745 open until the fix is released.

@dpiet-oai dpiet-oai closed this Sep 29, 2026
sylvesterkaczmarek added a commit to sylvesterkaczmarek/openai-java that referenced this pull request Sep 29, 2026
Non-2xx responses bypass the service-level cleanup when the shared error
handler throws. Close those responses inside the handler while leaving
successful responses open for their callers. Preserve existing exception
types, status codes, headers, and parsed error details; failures during
cleanup remain suppressed on the original exception.

Combines the cleanup implementation from openai#746 and openai#879, openai#746's real
error-body parsing and exception-detail assertions, and openai#879's coverage
across every exception branch. Additional tests cover status boundaries,
malformed JSON, and failures in both parsing and cleanup.

Thanks to @fallintoplace and @sylvesterkaczmarek for the original fixes.
Both contributors are credited in the commit. This maintainer-owned
replacement consolidates their work; the original PRs remain open
pending review.

Related issue: openai#745. Keep the issue open until a release containing the
fix is available.

Validation: `:openai-java-core:lintKotlin` and `:openai-java-core:test
--tests com.openai.core.handlers.ErrorHandlerTest --tests
com.openai.services.ErrorHandlingTest` passed with JDK 21: 37 tests,
zero failures (20 new handler cases and 17 existing service cases). `git
diff --check` also passed. The full test suite was not run.

Co-authored-by: Minh Vu <vuhoangminh97@gmail.com>
Co-authored-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Non-2xx responses are not closed when error handling throws

3 participants