refactor: replace reqwest with hyper-util and add client pooling

[quettabit]

No description provided.

[quettabit]

@greptileai review this draft

[greptile-apps]

Greptile Overview

Greptile Summary

Replaced the reqwest HTTP client with a custom hyper-util based implementation that adds connection pooling with per-host client management, concurrent request limiting (90 per client), and automatic idle client reaping (90s timeout).

Key Changes:

• Introduced new src/client.rs module with Pool, PooledClient, and RequestPermit for managing HTTP connections
• Refactored BaseClient in src/api.rs to use trait-based RequestExecutor instead of direct reqwest::Client
• Made error handlers synchronous (removed async from response processing)
• Updated dependency from reqwest 0.13 to hyper-util 0.1, hyper-rustls 0.27, and http-body-util 0.1
• Upgraded rand from 0.9 to 0.10 (updated trait imports from Rng to RngExt)
• Added comprehensive unit tests for pooling behavior (checkout, overflow, permit drops, reaping, host isolation)

The pooling implementation correctly tracks active requests via RequestPermit RAII guards that decrement counters on drop. Previous review comments about unary permits being dropped early were addressed by the developer clarifying that _permit bindings live until function scope end.

Confidence Score: 4/5

• This PR is safe to merge with low risk, pending standard testing
• Large refactoring replacing HTTP client library with custom pooling implementation. The code is well-structured with comprehensive unit tests covering pooling behavior. The developer has addressed previous concerns about permit handling. Minor risk due to the scope of changes affecting all HTTP operations, but the implementation follows solid patterns (RAII for resource management, trait-based abstractions).
• Pay close attention to src/client.rs and src/api.rs during integration testing to ensure pooling behavior works correctly under load

Important Files Changed

Filename	Overview
Cargo.toml	Replaced reqwest with hyper-util, hyper-rustls, and supporting crates; upgraded rand to 0.10; added async-trait, serde_urlencoded, and tokio sync feature
src/client.rs	New module implementing hyper-based HTTP client with connection pooling (90 concurrent requests/client), request permits, idle client reaping (90s timeout), compression support, and streaming/unary response handling
src/api.rs	Refactored to use new client module instead of reqwest; made error handlers synchronous; removed async from response processing; updated BaseClient to use trait-based RequestExecutor
src/ops.rs	Updated S2::new() to create BaseClient before AccountClient; added hidden new_with_connector() for testing with custom connectors

Sequence Diagram

sequenceDiagram
    participant App as Application
    participant API as api.rs (BaseClient)
    participant Pool as client::Pool
    participant HostPool as client::HostPool
    participant Client as HyperClient
    participant Server as S2 Server

    App->>API: execute_unary(request)
    API->>Pool: checkout(host)
    Pool->>HostPool: get_or_create_host_pool(host)
    HostPool->>HostPool: check existing clients for capacity
    alt Client has capacity
        HostPool->>HostPool: request_permit() ✓
        HostPool-->>Pool: (client, permit)
    else All clients full
        HostPool->>HostPool: create_client()
        HostPool->>HostPool: request_permit() ✓
        HostPool-->>Pool: (new_client, permit)
    end
    Pool-->>API: (client, _permit)
    API->>Client: request(http_request)
    Client->>Server: HTTP Request
    Server-->>Client: HTTP Response
    Client-->>API: UnaryResponse
    API->>API: decompress_body()
    API-->>App: Result<UnaryResponse>
    Note over API,Pool: _permit dropped, active_requests--

    App->>API: init_streaming(request)
    API->>Pool: checkout(host)
    Pool->>HostPool: checkout()
    HostPool-->>Pool: (client, permit)
    Pool-->>API: (client, permit)
    API->>Client: request(http_request)
    Client->>Server: HTTP Request (streaming)
    Server-->>Client: HTTP Response Stream
    Client-->>API: StreamingResponse{body, permit}
    API-->>App: StreamingResponse
    Note over App,API: permit held until stream consumed
    App->>App: consume stream frames
    App->>App: drop StreamingResponse
    Note over API,Pool: permit dropped, active_requests--

Loading

[greptile-apps]

_{9 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[quettabit]

@greptileai review this draft again and list what you'd like to see to improve your confidence

[greptile-apps]

Based on reviewing this draft PR (1,300 additions, 705 deletions across 8 files), here's what would improve my confidence:

High Priority

1. PR description - Currently just "[WIP]". Need to understand:

   • What problem this solves
   • Why you're implementing a custom hyper connection pool
   • Performance/reliability goals
   • Breaking changes (if any)

2. Test coverage - For a change this size touching core networking:

   • Unit tests for the new pool implementation
   • Integration tests showing connection reuse works correctly
   • Edge case tests (connection failures, pool exhaustion, concurrent access)
   • Performance benchmarks comparing old vs new approach

3. Error handling verification - Since this involves connection pooling:

   • How stale connections are detected and removed
   • Behavior when pool is exhausted
   • Retry/backoff strategies for failed connections

Medium Priority

4. Documentation - For the new components:

   • Rustdoc comments on public APIs
   • Configuration options and their defaults
   • Migration guide if there are API changes

5. CI status - The PR shows mergeable_state: "blocked":

   • All tests passing
   • No clippy warnings
   • Formatting checks passing

6. Code review of critical paths:

   • Connection lifecycle management
   • Thread safety of the pool
   • Resource cleanup (no connection leaks)

Would Be Nice

7. Performance data - Before/after metrics showing the improvement
8. Changelog entry - If this is user-facing
9. Examples updated - If the API changed

The resolved comment about async-compression downgrade suggests you're managing dependency conflicts deliberately - that kind of rationale would be helpful in the PR description for other intentional choices.

[quettabit]

@greptileai can you update your review as files have changed since your first review

[greptile-apps]

_{8 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[quettabit]

@greptileai i've responded to your comments. can you review again?

[greptile-apps]

_{9 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[quettabit]

@greptileai i've responded to your comments. can you review again?

[greptile-apps]

_{9 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

src/api.rs
Overbroad retry classification

ClientError::is_retryable returns true for everything except Others, which makes Timeout, RequestCanceled, and ConnectionClosedEarly retryable. With the new client::Error mapping, RequestCanceled can occur when the client-side timeout fires (tokio::time::timeout aborts the request future), so timeouts/cancels may be retried even when they are deterministic under current load/timeout settings. This changes retry behavior vs. the prior reqwest-based classification; consider tightening is_retryable to avoid retrying timeouts/cancellations if that’s not intended for this SDK.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/api.rs
Line: 569:572

Comment:
**Overbroad retry classification**

`ClientError::is_retryable` returns `true` for everything except `Others`, which makes `Timeout`, `RequestCanceled`, and `ConnectionClosedEarly` retryable. With the new `client::Error` mapping, `RequestCanceled` can occur when the client-side timeout fires (`tokio::time::timeout` aborts the request future), so timeouts/cancels may be retried even when they are deterministic under current load/timeout settings. This changes retry behavior vs. the prior reqwest-based classification; consider tightening `is_retryable` to avoid retrying timeouts/cancellations if that’s not intended for this SDK.

How can I resolve this? If you propose a fix, please make it concise.

[quettabit]

@greptileai is_retryable is fine and i've responded re: your comment on permit drop. pls review again and update your score, and also pls don't repeat your comments. just evaluate based on my responses.