Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: add Clone support and HTTP client pooling for providers#100

Open
nazq wants to merge 1 commit intograniet:main from
nazq:feat/client-pooling-clone
Open

feat: add Clone support and HTTP client pooling for providers #100
nazq wants to merge 1 commit intograniet:main from
nazq:feat/client-pooling-clone

Conversation

@nazq
Copy link
Contributor

@nazq nazq commented Dec 30, 2025

Summary

  • Add Clone derive to all provider structs, enabling shared ownership
  • Add with_client() constructors for HTTP client injection and connection pooling
  • Add .client() builder method for LLMBuilder
  • Fix ElevenLabs to use consistent builder pattern with timeout support

Closes #91

Motivation

As noted in #91, creating multiple provider instances currently means each one allocates its own reqwest::Client and connection pool. This is not ideal when users want to:

  1. Share a single connection pool across multiple providers
  2. Clone provider instances for concurrent use
  3. Use custom client configurations (proxies, custom TLS, etc.)

Approach

Non-breaking addition: Rather than modifying the existing new() constructors (which would be a breaking change), I added a parallel with_client() constructor to each backend. This preserves backward compatibility.

If preferred, a future major version could consolidate these into new() with Option<Client>. Happy to make the change if agreed.

Changes

Clone Support

All provider structs now derive Clone:

  • Anthropic, OpenAI, Google, Ollama, DeepSeek, XAI, AzureOpenAI, Phind, ElevenLabs
  • OpenAICompatibleProvider<T> (and config structs: MistralConfig, GroqConfig, etc.)

This works because reqwest::Client is internally Arc-wrapped, making clones cheap.

Client Injection

New with_client() constructors on all backends:

// Share a single client across providers
let shared_client = reqwest::Client::builder()
 .timeout(Duration::from_secs(60))
 .pool_max_idle_per_host(10)
 .build()?;
let anthropic = Anthropic::with_client(
 shared_client.clone(),
 api_key,
 // ... other params
);
let openai = OpenAI::with_client(
 shared_client.clone(), // Same connection pool
 api_key,
 // ... other params
)?;

Builder Support

New .client() method on LLMBuilder:

let shared_client = reqwest::Client::builder()
 .timeout(Duration::from_secs(60))
 .build()?;
let openai = LLMBuilder::new()
 .backend(LLMBackend::OpenAI)
 .client(shared_client.clone())
 .api_key("...")
 .build()?;
let anthropic = LLMBuilder::new()
 .backend(LLMBackend::Anthropic)
 .client(shared_client) // Shared pool
 .api_key("...")
 .build()?;

Test Plan

  • cargo check passes
  • cargo clippy passes
  • cargo test - 31/31 unit tests pass (6 new tests added)
  • cargo build --release passes

New Tests Added

Test Location Purpose
test_anthropic_clone src/backends/anthropic.rs Verify Anthropic can be cloned
test_anthropic_with_client src/backends/anthropic.rs Verify Anthropic works with injected client
test_openai_compatible_provider_clone src/providers/openai_compatible.rs Verify OpenAICompatibleProvider can be cloned
test_openai_compatible_provider_with_client src/providers/openai_compatible.rs Verify OpenAICompatibleProvider works with injected client
test_builder_client_method src/builder.rs Verify LLMBuilder.client() sets client
test_builder_without_client src/builder.rs Verify builder works without client (default)

@nazq nazq mentioned this pull request Dec 30, 2025
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

i️ 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".

Comment on lines +506 to +507
pub fn client(mut self, client: reqwest::Client) -> Self {
self.client = Some(client);
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Dec 30, 2025

Choose a reason for hiding this comment

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

P2 Badge Apply builder client to all backends or document limits

When a caller sets .client(...) and then builds any backend other than OpenAI/Anthropic, the custom reqwest::Client is silently ignored because LLMBuilder::build only consults self.client in those two branches and all other backends still call their new/with_config constructors with fresh clients. This means connection pooling/proxy/TLS/timeouts supplied via .client() never take effect for providers like Google/Ollama/DeepSeek, which violates the new method’s documented behavior and can lead to unexpected networking behavior for those backends.

Useful? React with 👍 / 👎.

nazq reacted with thumbs up emoji
@nazq nazq force-pushed the feat/client-pooling-clone branch from a382292 to 3826f38 Compare December 30, 2025 15:13
@nazq nazq force-pushed the feat/client-pooling-clone branch from 3826f38 to b991514 Compare December 31, 2025 16:50
Copy link
Contributor

Hi, really appreciate the PR, but there is one significant problem. I see that you have added #[derive(Clone)] on structs. Which is a quick non breaking change but it also forces the internal structure elements/fields to be clone. Since the structures include a lot of heap allocated items (strings) this also clones them and could be more expensive then creating a new reqwest::Client.

Copy link
Contributor

I think if we delete the #[derive(Clone)] and keep with_client.

let people decide and wrap the structs in Arc<Backend> or Rc<Backend> and only take immutable references to &dyn Backend, things should work.

thoughts @nazq ?

Copy link
Contributor

aniketfuryrocks commented Jan 5, 2026
edited
Loading

I always implement everything from scratch. this was my claude implementation, adding an init_with_client should help sharing and keeping configs to structs wrapped in Arc cause they don't change. init honestly has to do nothing as there is no protocol level handshake or an open connection required. Also notice Cow, since you need to serialize anyways when sending requests, preventing string clone within the codebase should be a look out.

use std::{borrow::Cow, sync::Arc};
use crate::claude::config::ClaudeConfig;
use crate::claude::types::{ClaudeMessage, ClaudeRequest, ClaudeResponse};
use reqwest::header::HeaderMap;
const API_BASE: &str = "https://api.anthropic.com/v1";
#[derive(Clone)]
pub struct ClaudeApiClient {
 client: reqwest::Client,
 config: Arc<ClaudeConfig>,
}
impl ClaudeApiClient {
 pub fn init(config: ClaudeConfig) -> anyhow::Result<Self> {
 let mut headers = HeaderMap::new();
 // Add authentication
 headers.insert("x-api-key", config.api_key.parse()?);
 headers.insert("content-type", "application/json".parse().unwrap());
 headers.insert("anthropic-version", "2023-06-01".parse().unwrap());
 // reqwest client
 let client = reqwest::Client::builder()
 .default_headers(headers)
 .build()?;
 // self
 Ok(Self {
 client,
 config: Arc::new(config),
 })
 }
 pub async fn send_message(
 &self,
 tools: &[serde_json::Value],
 messages: Vec<ClaudeMessage<'_>>,
 system_prompt: &str,
 ) -> Result<ClaudeResponse<'static>, tonic::Status> {
 // Create the request from the config
 let request = ClaudeRequest {
 model: Cow::Borrowed(self.config.model),
 max_tokens: self.config.max_tokens,
 messages,
 system: Some(Cow::Borrowed(system_prompt)),
 temperature: self.config.temperature,
 top_p: self.config.top_p,
 top_k: self.config.top_k,
 tools: Some(Cow::Borrowed(tools)),
 };
 // Send the request
 let res = self
 .client
 .post(format!("{}/messages", API_BASE))
 .json(&request)
 .send()
 .await
 .map_err(|e| {
 tracing::warn!("Failed to send request to Claude API: {}", e);
 tonic::Status::unavailable("Failed to connect to Claude API")
 })?;
 // Parse if success
 let status_code = res.status();
 if status_code.is_success() {
 let text = res.text().await.map_err(|e| {
 tracing::warn!("Failed to read response text: {}", e);
 tonic::Status::internal("Failed to read response text")
 })?;
 return serde_json::from_str::<ClaudeResponse<'static>>(&text)
 .inspect_err(|err| tracing::warn!("Failed to parse Claude api response {err}"))
 .map_err(|_| tonic::Status::internal("Failed to parse Claude api response"));
 }
 let error_text = res.text().await.unwrap_or_else(|e| {
 tracing::warn!("Failed to read error response: {}", e);
 "Unknown error".to_string()
 });
 // Log the full error details internally
 tracing::error!(
 status_code = %status_code,
 response_body = %error_text,
 model = %self.config.model,
 "Claude API request failed"
 );
 // Map to appropriate tonic::Status based on status code
 match status_code.as_u16() {
 429 => Err(tonic::Status::resource_exhausted(
 "Claude API Rate limit exceeded",
 )),
 500..=599 => Err(tonic::Status::unavailable("Claude API is unavailable")),
 _ => Err(tonic::Status::internal("Claude API error")),
 }
 }
}
#[derive(Debug, serde::Serialize, serde::Deserialize, Default)]
pub struct ClaudeConfig {
 pub api_key: Cow<'static, str>,
 pub model: &'static str,
 pub max_tokens: u32,
 pub temperature: Option<f32>,
 pub top_p: Option<f32>,
 pub top_k: Option<u32>,
}

Copy link
Contributor Author

nazq commented Jan 5, 2026

I like the

I think if we delete the #[derive(Clone)] and keep with_client.

let people decide and wrap the structs in Arc<Backend> or Rc<Backend> and only take immutable references to &dyn Backend, things should work.

thoughts @nazq ?

I think this makes more sense than my approach. @graniet at some point it may be worth thinking about a breaking change release and brainstorm any API changes you may have in mind.

aniketfuryrocks reacted with thumbs up emoji aniketfuryrocks reacted with eyes emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow Clone

AltStyle によって変換されたページ (->オリジナル) /