feat: implement shared reqwest::Client for improved retry policies and DOS protection #1285
Conversation
|
@oscartbeaumont can you please review this PR |
WalkthroughAdds a lazily-initialized global reqwest::Client with retry policies, replaces local client construction across captions/upload/web_api to use the shared client (adjusting web_api builder signatures to accept Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (captions / upload / web_api)
participant Shared as shared_client
participant Lock as OnceLock
participant HTTP as reqwest::Client
Caller->>Shared: get_shared_client() / get_retryable_client(host)
alt first initialization
Shared->>Lock: get_or_init()
Lock->>HTTP: build Client (timeout=30s, retry policy)
HTTP-->>Lock: Client instance
Lock-->>Shared: &'static Client
else cached
Lock-->>Shared: &'static Client
end
Shared-->>Caller: &'static Client
Caller->>HTTP: perform request using shared client
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/shared_client.rs (1)
11-18: Consider adding connection pool configuration.The shared client uses default connection pooling settings. For a desktop application making concurrent uploads/downloads, you might want to tune these parameters.
pub fn get_shared_client() -> &'static Client { SHARED_CLIENT.get_or_init(|| { Client::builder() + .pool_max_idle_per_host(10) + .pool_idle_timeout(Some(std::time::Duration::from_secs(90))) .timeout(std::time::Duration::from_secs(30)) .build() .expect("Failed to create shared HTTP client") }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src-tauri/src/captions.rs(2 hunks)apps/desktop/src-tauri/src/lib.rs(1 hunks)apps/desktop/src-tauri/src/shared_client.rs(1 hunks)apps/desktop/src-tauri/src/upload.rs(3 hunks)apps/desktop/src-tauri/src/web_api.rs(5 hunks)packages/ui-solid/src/auto-imports.d.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/shared_client.rsapps/desktop/src-tauri/src/web_api.rsapps/desktop/src-tauri/src/upload.rsapps/desktop/src-tauri/src/captions.rs
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
packages/ui-solid/src/auto-imports.d.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
packages/ui-solid/src/auto-imports.d.ts
🧬 Code graph analysis (3)
apps/desktop/src-tauri/src/web_api.rs (1)
apps/desktop/src-tauri/src/shared_client.rs (1)
get_shared_client(11-18)
apps/desktop/src-tauri/src/upload.rs (1)
apps/desktop/src-tauri/src/shared_client.rs (2)
get_retryable_client(50-52)get_retryable_client_builder(25-44)
apps/desktop/src-tauri/src/captions.rs (1)
apps/desktop/src-tauri/src/shared_client.rs (1)
get_shared_client(11-18)
🔇 Additional comments (4)
apps/desktop/src-tauri/src/lib.rs (1)
24-24: LGTM: Module declaration is correct.The
shared_clientmodule declaration is properly positioned and follows Rust naming conventions.apps/desktop/src-tauri/src/web_api.rs (1)
61-80: LGTM: Proper client centralization.The changes correctly centralize HTTP client usage and improve the API by making the client dependency explicit through the function signature. This also improves testability.
apps/desktop/src-tauri/src/captions.rs (1)
1072-1077: Review concern about 30-second timeout is valid but requires design decision.The shared client is configured with a 30-second timeout, and model file sizes range from 75 MB (tiny) to 3.1 GB (large-v3). For a 466 MB file (small model) at typical home broadband speeds (10 Mbps), the download alone would require ~373 seconds, far exceeding the 30-second timeout. At 100 Mbps (fiber), even then most models exceed the timeout window.
The download function currently has no retry mechanism or streaming with resume capability. Any timeout will immediately fail with an error message rather than retry or resume.
This is a legitimate concern IF your target users download medium-to-large models. However, this may be intentional by design (restricting to tiny/base models, or requiring users with fast connections). You should verify:
- What model size(s) do you expect users to download?
- What is your minimum target internet speed?
- Should larger models use a longer timeout, resumable downloads, or background service?
apps/desktop/src-tauri/src/shared_client.rs (1)
25-44: The review comment is incorrect.The
reqwest::retrymodule withfor_host()function is a valid API in reqwest 0.12, wherefor_host()returns a retry Builder that can be configured and attached to a ClientBuilder. The code uses the correct pattern and will compile successfully. No code changes are required, and no additional dependencies need to be added toCargo.toml.The codebase already has the correct dependency (
reqwest = { version = "0.12.7", ... }), and the implementation inshared_client.rsproperly uses the retry builder API with appropriate configuration methods like.classify_fn(),.max_retries_per_request(), and.max_extra_load().Likely an incorrect or invalid review comment.
…pload functions to utilize it
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/shared_client.rs (1)
36-42: Consider removing the unused_hostparameter.The function signature suggests host-specific behavior, but the
_hostparameter is ignored and the same shared client is returned for all hosts. This creates a misleading API that may confuse callers.Consider one of these options:
- Preferred: Remove the parameter entirely if host-specific clients aren't needed:
-pub fn get_retryable_client(_host: String) -> Result<&'static Client, reqwest::Error> { +pub fn get_retryable_client() -> &'static Client { - Ok(get_shared_client()) + get_shared_client() }
- Alternative: If keeping for API compatibility, add a comment explaining why the parameter exists but is unused:
+/// Note: The `host` parameter is currently unused but retained for API compatibility. +/// All requests share the same client instance for consistent DOS protection. pub fn get_retryable_client(_host: String) -> Result<&'static Client, reqwest::Error> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/shared_client.rs(1 hunks)apps/desktop/src-tauri/src/upload.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/upload.rsapps/desktop/src-tauri/src/shared_client.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/upload.rs (1)
apps/desktop/src-tauri/src/shared_client.rs (1)
get_retryable_client(40-42)
🔇 Additional comments (3)
apps/desktop/src-tauri/src/shared_client.rs (1)
11-34: LGTM! Shared client addresses previous architectural concern.The single shared client with
for_all_hosts()retry policy now provides the global request tracking mentioned in the PR objectives. This design properly addresses the previous review concern about per-host clients breaking global tracking.apps/desktop/src-tauri/src/upload.rs (2)
715-722: Previous concern addressed: proper host validation.The code now correctly returns an error when the host is missing instead of using a
"<unknown>"fallback. This ensures per-host retry limiting works as intended.
801-806: Previous concern addressed: proper host validation (singlepart).Similar to the multipart uploader, this code now correctly returns an error when the host is missing, ensuring consistent behavior across both upload paths.
…edundant builder function
…ty in upload module
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/shared_client.rs(1 hunks)apps/desktop/src-tauri/src/upload.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src-tauri/src/upload.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/shared_client.rs
🔇 Additional comments (1)
apps/desktop/src-tauri/src/shared_client.rs (1)
1-5: LGTM! Idiomatic use ofOnceLockfor thread-safe lazy initialization.The use of
OnceLockfor a globally shared client is the modern, idiomatic approach in Rust.
| pub fn get_shared_client() -> &'static Client { | ||
| SHARED_CLIENT.get_or_init(|| { | ||
| Client::builder() | ||
| .retry( | ||
| reqwest::retry::for_all() | ||
| .classify_fn(|req_rep| { | ||
| match req_rep.status() { | ||
| // Server errors and rate limiting | ||
| Some(s) | ||
| if s.is_server_error() | ||
| || s == reqwest::StatusCode::TOO_MANY_REQUESTS => | ||
| { | ||
| req_rep.retryable() | ||
| } | ||
| // Network errors | ||
| None => req_rep.retryable(), | ||
| _ => req_rep.success(), | ||
| } | ||
| }) | ||
| .max_retries_per_request(5) | ||
| .max_extra_load(5.0), | ||
| ) | ||
| .timeout(std::time::Duration::from_secs(30)) | ||
| .build() | ||
| .expect("Failed to create shared HTTP client") | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the reqwest::retry::for_all() API and consider error handling.
The retry configuration looks reasonable, but I'd like to verify the API usage:
-
The
reqwest::retry::for_all()call on line 15 should be confirmed to exist in the version ofreqwestbeing used. Standardreqwestdoesn't include a built-in retry module by default—this typically requires thereqwest-retrycrate or similar middleware. -
The
.expect()on line 35 will panic if client creation fails during initialization. While this may be acceptable for a critical shared resource, consider whether a more graceful startup failure would be appropriate.
Run the following script to verify the reqwest retry API:
Also, please verify the retry API documentation:
🏁 Script executed:
#!/bin/bash
# Check Cargo.toml for reqwest and retry-related dependencies
rg -n "reqwest" apps/desktop/src-tauri/Cargo.toml -A 3Length of output: 361
🌐 Web query:
reqwest retry for_all method documentation
💡 Result:
Do you mean the built-in reqwest retry API (reqwest::retry) or the reqwest-retry middleware (crate reqwest-retry)? Which crate and version are you using?
For reference:
- reqwest retry module docs: https://docs.rs/reqwest/latest/reqwest/retry. [1]
- reqwest-retry middleware docs: https://docs.rs/reqwest-retry/latest/reqwest_retry. [2]
Reply with which one (and the version) and I’ll find the exact for_all documentation or show how to use it.
Excellent! I found critical information. The retry module was added to reqwest via PR #2763, which includes a new ClientBuilder::retry() method and a reqwest::retry module to build a retry policy, with a for_host() function and classify_fn() method. However, I need to verify when this was released and whether it's in reqwest 0.12.7.Perfect! I found it. As of version 0.12.23, reqwest includes automatic retry functionality - which means reqwest 0.12.7 does NOT have the built-in retry module. The PR #2763 added it, but it was only released in 0.12.23 or later.
Let me verify the exact API by checking if for_all() is the correct method name:
Looking at the search results, I found a critical issue. The code uses reqwest::retry::for_all(), but the built-in retry functionality was only added to reqwest starting in version 0.12.23, and the version specified in Cargo.toml is 0.12.7. Additionally, the API examples show for_host(), not for_all(). Let me check the actual imports in the source file to be certain:Based on my research, I now have sufficient evidence to write the rewritten review comment. The critical issue is that reqwest's automatic retry functionality was only added in version 0.12.23, but the codebase uses reqwest 0.12.7. The code calls reqwest::retry::for_all(), which does not exist in that version and would not compile.
Code will not compile with reqwest 0.12.7—retry module is unavailable.
The code uses reqwest::retry::for_all() on line 15, but automatic retry functionality was added to reqwest only in version 0.12.23. The Cargo.toml specifies reqwest 0.12.7, which does not include the reqwest::retry module.
Upgrade reqwest to at least 0.12.23 to enable the built-in retry API. Additionally, verify that the for_all() method is the correct API (the PR documentation references for_host() for host-specific retry policies).
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/shared_client.rs around lines 11 to 37, the code
calls reqwest::retry::for_all() but the project’s Cargo.toml pins reqwest to
0.12.7 which does not include the retry module (added in >=0.12.23) and may use
a different API (examples show for_host()). Fix by updating Cargo.toml to
reqwest >=0.12.23 (or latest 0.12.x that includes retry), run cargo update, and
then verify/adjust the retry call to the correct API (e.g., use for_host() or
the actual factory function provided by that reqwest version) so the code
compiles.
| /// Get a retryable client for specific hosts | ||
| /// | ||
| /// This function returns the shared client which has global retry tracking. | ||
| /// All requests use the same client instance for consistent DOS protection. | ||
| pub fn get_retryable_client(_host: String) -> Result<&'static Client, reqwest::Error> { | ||
| Ok(get_shared_client()) | ||
| } |
There was a problem hiding this comment.
Misleading API: unused parameter and infallible Result type.
The function signature has two issues that make the API confusing:
-
The
_hostparameter is completely unused (indicated by the underscore prefix). If the host information isn't needed, this parameter should be removed. -
The function returns
Result<&'static Client, reqwest::Error>but can never return anErrvariant—it always returnsOk(get_shared_client()). This is misleading to callers who might write error-handling code that will never execute.
Consider one of these approaches:
Option 1 (Recommended): Simplify the API
If the host parameter truly isn't needed, remove it and change the return type:
-pub fn get_retryable_client(_host: String) -> Result<&'static Client, reqwest::Error> {
- Ok(get_shared_client())
+pub fn get_retryable_client() -> &'static Client {
+ get_shared_client()
}Then update callers in upload.rs to not pass the host parameter.
Option 2: Remove the wrapper entirely
If get_retryable_client is just an alias for get_shared_client, consider removing it and having callers use get_shared_client() directly.
Option 3: Document the intent
If the signature is intentionally kept for future extensibility or API stability, add a comment explaining why the parameter is unused and why a Result is returned.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/shared_client.rs around lines 39–45, the function
get_retryable_client currently accepts an unused _host: String and returns a
Result that is never Err; remove the unused parameter and simplify the signature
to return &'static Client (no Result), i.e. pub fn get_retryable_client() ->
&'static Client { get_shared_client() }, then update all callers (e.g., in
upload.rs) to stop passing a host and to handle the direct client return;
alternatively if you prefer removing the wrapper, replace calls to
get_retryable_client() with get_shared_client() instead.
|
I'm closing this as:
Thanks though! |
2 participants
Implements a shared
reqwest::Clientinstance to centralize HTTP client usage across the desktop app.Changes:
shared_client.rsmodule with global client and host-specific retryable clientsweb_api.rs,upload.rs, andcaptions.rsto use shared clientBenefits:
Closes #1135
Summary by CodeRabbit