Skip to content

Fixed leaking Guzzle exceptions from the HTTP client - #12

Merged
tg666 merged 1 commit into
mainfrom
fix/unexpected-guzzle-errors
Nov 10, 2025
Merged

Fixed leaking Guzzle exceptions from the HTTP client#12
tg666 merged 1 commit into
mainfrom
fix/unexpected-guzzle-errors

Conversation

@tg666

@tg666 tg666 commented Nov 10, 2025

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Bug Fixes
    • Fixed HTTP client exceptions (e.g., connection errors) leaking to callers. Low-level transport errors are now consistently caught and surfaced as a standardized unexpected-error response, preventing internal implementation details from reaching client code and improving stability.

@coderabbitai

coderabbitai Bot commented Nov 10, 2025

Copy link
Copy Markdown

Walkthrough

Adds a changelog entry for v1.5.1 and updates HTTP middleware to map promise rejections via .otherwise(), converting non-Throwable rejections to Exception('Unknown error.') and ensuring rethrown errors are AmpExceptionInterface or UnexpectedErrorException.

Changes

Cohort / File(s) Change Summary
Release documentation
CHANGELOG.md
Added a new release block [1.5.1] - 2025-11-10 noting a fix: leaking Guzzle exceptions (e.g., ConnectException) are now wrapped as UnexpectedErrorException.
Error-wrapping middleware
src/Http/Middleware/UnexpectedErrorMiddleware.php
Added Exception import; added .otherwise() handler to map promise rejections: non-Throwable rejections are wrapped in Exception('Unknown error.'); rethrows AmpExceptionInterface as-is or wraps other throwables in UnexpectedErrorException.

Sequence Diagram

sequenceDiagram
    actor Client
    participant Middleware as UnexpectedErrorMiddleware
    participant Next as next() Promise
    participant Otherwise as .otherwise() Handler

    Client->>Middleware: handle(request)
    Middleware->>Next: call next()
    
    alt Promise resolves
        Next-->>Client: response
    else Promise rejects or throws
        Next-->>Otherwise: rejection (maybe non-Throwable or Throwable)
        Otherwise->>Otherwise: if not Throwable -> wrap in Exception('Unknown error.')
        Otherwise->>Middleware: throw mapped Throwable
        alt AmpExceptionInterface
            Middleware-->>Client: rethrow AmpExceptionInterface
        else other Throwable
            Middleware-->>Client: wrap in UnexpectedErrorException and throw
        end
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Inspect .otherwise() rejection mapping and conversion of non-Throwable values to Exception('Unknown error.')
  • Verify outer catch preserves and rethrows AmpExceptionInterface and wraps other throwables as UnexpectedErrorException
  • Confirm changelog formatting and date

Poem

🐰 A Guzzle leak hopped out of sight,
I caught it quick in middleware light.
Wrapped in errors neat and trim,
No raw exceptions hop at whim.
Hooray — the HTTP path is tight! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately describes the main change: fixing leaking Guzzle exceptions from the HTTP client by introducing proper exception handling.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/unexpected-guzzle-errors

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 31b360c and 38148c2.

📒 Files selected for processing (2)
  • CHANGELOG.md (1 hunks)
  • src/Http/Middleware/UnexpectedErrorMiddleware.php (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (1)
src/Http/Middleware/UnexpectedErrorMiddleware.php (1)
src/Exception/UnexpectedErrorException.php (1)
  • UnexpectedErrorException (10-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Unit Tests [PHP 8.1]
🔇 Additional comments (2)
src/Http/Middleware/UnexpectedErrorMiddleware.php (2)

8-8: LGTM: Required import for error handling.

The Exception import is necessary for handling non-Throwable promise rejections on line 32.


31-35: Test coverage for the async .otherwise() callback is missing.

The existing tests (testAmpExceptionShouldBeThrown and testUnexpectedErrorExceptionShouldBeThrown) only exercise the synchronous catch path. The async promise rejection handler (lines 31-35) is not covered by tests. This includes:

  • Non-Throwable promise rejections (line 32: conversion to Exception('Unknown error.'))
  • Promise rejections with AmpExceptionInterface instances (line 34: pass-through)
  • Promise rejections with other Throwable types (line 34: wrap in UnexpectedErrorException)

The code changes are logically correct and consistent with the sync path, but the async paths need test verification before merging.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coveralls

coveralls commented Nov 10, 2025

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 19223117614

Details

  • 1 of 4 (25.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 93.321%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Http/Middleware/UnexpectedErrorMiddleware.php 1 4 25.0%
Totals Coverage Status
Change from base Build 15209567259: -0.2%
Covered Lines: 1537
Relevant Lines: 1647

💛 - Coveralls

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5db4090 and 31b360c.

📒 Files selected for processing (2)
  • CHANGELOG.md (1 hunks)
  • src/Http/Middleware/UnexpectedErrorMiddleware.php (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Http/Middleware/UnexpectedErrorMiddleware.php (1)
src/Exception/UnexpectedErrorException.php (1)
  • UnexpectedErrorException (10-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Unit Tests [PHP 8.3]
  • GitHub Check: Php-Cs-Fixer
🔇 Additional comments (2)
CHANGELOG.md (1)

10-12: LGTM! Clear changelog entry.

The changelog entry accurately documents the fix and follows the Keep a Changelog format.

src/Http/Middleware/UnexpectedErrorMiddleware.php (1)

8-8: LGTM! Import is necessary.

The Exception import is required for the fallback case in the promise rejection handler where the rejection value might not be a Throwable.

Comment thread src/Http/Middleware/UnexpectedErrorMiddleware.php Outdated
@tg666
tg666 force-pushed the fix/unexpected-guzzle-errors branch from 31b360c to 38148c2 Compare November 10, 2025 06:44
@tg666
tg666 merged commit ef3d6f6 into main Nov 10, 2025
13 of 14 checks passed
@tg666
tg666 deleted the fix/unexpected-guzzle-errors branch November 10, 2025 06:52
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.

2 participants