Shared RaindexClient: eliminate per-request thread spawning#58
Open
Shared RaindexClient: eliminate per-request thread spawning#58
Conversation
RaindexClient is now Send+Sync on native (uses Arc<Mutex<...>> internally) and new_native() starts the sync scheduler internally.
Replace per-request thread spawning with a single client created at startup. Thread spawning is kept only for construction (DotrainRegistry future is not Send due to JsValue). Sync is now handled internally by RaindexClient::new_native(), so sync.rs is gutted.
Change async_trait(?Send) to async_trait and add Send + Sync bounds on OrderDataSource and SwapDataSource traits, since RaindexClient is now Send+Sync and handlers no longer need non-Send futures.
Replace run_with_client pattern with direct raindex.client() access in all route handlers. Remove per-request thread spawning overhead.
Remove tests for per-request client init failures (no longer possible since client is created eagerly at startup). Update mock helpers.
📝 WalkthroughWalkthroughThis PR refactors RaindexProvider to initialize and own a RaindexClient directly during load(), replacing the previous async run_with_client() helper pattern with direct client() accessor methods. The load() signature now accepts an optional db_path parameter. Trait bounds for OrderDataSource and SwapDataSource now require Send+Sync. The start_sync() function is removed as sync is now handled internally by RaindexClient. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.rs`:
- Around line 237-242: Before calling raindex::RaindexProvider::load (the block
that assigns raindex_config), validate local_db_path: ensure it is either an
absolute path or includes a directory component (i.e.,
Path::new(&local_db_path).is_absolute() ||
Path::new(&local_db_path).parent().and_then(|p| p.to_str()).filter(|s|
!s.is_empty()).is_some()). If the check fails (bare filename like "local.db"),
return or propagate an error / log and exit prior to invoking
RaindexProvider::load so the code does not forward invalid paths; reference the
raindex_config assignment, the local_db_path variable, and
raindex::RaindexProvider::load when adding the guard.
In `@src/raindex/config.rs`:
- Around line 14-20: The load function should reject bare filenames for db_path
at load-time: in pub(crate) async fn load(...), if let Some(db) =
db_path.clone() { check db.parent() and ensure it is Some and not empty (i.e.,
path has a directory prefix like "./data/local.db" or "db/local.db"); if parent
is None or empty, return an Err(RaindexProviderError::InvalidConfig(...)) (or
add a new RaindexProviderError variant) with a clear message so consumers cannot
pass bare "local.db". Implement this validation early in load before using db.
In `@src/routes/order/deploy_dca.rs`:
- Around line 34-35: The handler currently calls todo!() after acquiring
_raindex (shared_raindex.read().await) which panics and bypasses the ApiError
contract; replace the todo!() with proper non-panicking error handling that
returns a Result with an ApiError variant (e.g.,
Err(ApiError::InternalServerError or a more specific ApiError::BadRequest)
rather than panicking), ensure the function (deploy_dca handler) returns the
expected Result type, and propagate or map any errors from shared_raindex access
or subsequent logic into ApiError so all failures go through the ApiError enum.
In `@src/routes/order/deploy_solver.rs`:
- Around line 34-35: The handler currently calls todo!() (after awaiting
shared_raindex.read().await) which panics on every /v1/order/solver request;
replace the panic with a proper ApiError return so the error flows through typed
API error handling. Specifically, remove todo!() and return an
Err(ApiError::...) from the handler (e.g., ApiError::Internal or a more specific
variant like ServiceUnavailable) with a clear message such as "solver endpoint
not implemented" and include any context you want from shared_raindex; ensure
the function signature (the route handler that awaited
shared_raindex.read().await) still returns a Result so the Err variant is used
instead of panicking.
In `@src/routes/orders.rs`:
- Around line 37-38: The handlers currently call todo!() after acquiring the
read lock on shared_raindex (let _raindex = shared_raindex.read().await) which
panics and breaks the endpoints; replace todo!() in both order handlers with
real logic that uses the acquired read guard (use the variable name e.g.,
raindex) to perform the lookup/operation and return Result<..., ApiError>,
mapping any failure to the appropriate ApiError variant instead of panicking or
returning raw status codes; ensure all error paths convert errors into ApiError
(via ? or map_err) and that the handler signatures return the ApiError-based
Response type so the endpoints return typed API errors rather than panics.
In `@src/routes/trades.rs`:
- Around line 37-38: The handlers for /v1/trades/tx/{tx_hash} and
/v1/trades/{address} currently call todo!() and thus panic; replace the todo!()
in the blocks that acquire shared_raindex.read().await with real logic that
queries the raindex and returns results or an ApiError variant instead of
panicking. Specifically, in the function(s) handling tx_hash and address use the
read guard from shared_raindex to perform the lookup, map a missing result to
the appropriate ApiError (e.g., ApiError::NotFound or the project's canonical
error variant) and map any internal errors to ApiError::Internal or
ApiError::ServiceUnavailable as appropriate, then return Ok(response) on
success; ensure every failure path returns Err(ApiError::...) so no raw status
codes or panics remain.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
lib/rain.orderbooksrc/main.rssrc/raindex/config.rssrc/routes/admin.rssrc/routes/order/cancel.rssrc/routes/order/deploy_dca.rssrc/routes/order/deploy_solver.rssrc/routes/order/get_order.rssrc/routes/order/mod.rssrc/routes/orders.rssrc/routes/swap/calldata.rssrc/routes/swap/mod.rssrc/routes/swap/quote.rssrc/routes/tests.rssrc/routes/trades.rssrc/sync.rssrc/test_helpers.rs
Comment on lines
+237
to
+242
| let raindex_config = match raindex::RaindexProvider::load( | ||
| ®istry_url, | ||
| Some(local_db_path), | ||
| ) | ||
| .await | ||
| { |
There was a problem hiding this comment.
Enforce explicit local_db_path format before loading Raindex provider.
Line 237 now always forwards local_db_path into RaindexProvider::load, but bare filenames (e.g., local.db) are still not rejected. Add an explicit validation guard before this call.
🔧 Proposed fix
let local_db_path = std::path::PathBuf::from(&cfg.local_db_path);
+let has_directory_prefix = local_db_path
+ .parent()
+ .map(|p| !p.as_os_str().is_empty())
+ .unwrap_or(false);
+if !has_directory_prefix {
+ tracing::error!(
+ path = %cfg.local_db_path,
+ "local_db_path must include a directory prefix (e.g. data/raindex.db)"
+ );
+ drop(log_guard);
+ std::process::exit(1);
+}
if let Some(parent) = local_db_path.parent() {Based on learnings: In ST0x-Technology/st0x.rest.api, configuration-related Rust code should use DB paths with a directory prefix (e.g., data/raindex.db) or absolute path, never bare filenames like local.db.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.rs` around lines 237 - 242, Before calling
raindex::RaindexProvider::load (the block that assigns raindex_config), validate
local_db_path: ensure it is either an absolute path or includes a directory
component (i.e., Path::new(&local_db_path).is_absolute() ||
Path::new(&local_db_path).parent().and_then(|p| p.to_str()).filter(|s|
!s.is_empty()).is_some()). If the check fails (bare filename like "local.db"),
return or propagate an error / log and exit prior to invoking
RaindexProvider::load so the code does not forward invalid paths; reference the
raindex_config assignment, the local_db_path variable, and
raindex::RaindexProvider::load when adding the guard.
Comment on lines
+14
to
+20
| pub(crate) async fn load( | ||
| registry_url: &str, | ||
| db_path: Option<PathBuf>, | ||
| ) -> Result<Self, RaindexProviderError> { | ||
| let url = registry_url.to_string(); | ||
| let (tx, rx) = tokio::sync::oneshot::channel::<Result<DotrainRegistry, WorkerError>>(); | ||
| let db = db_path.clone(); | ||
|
|
There was a problem hiding this comment.
Validate db_path format at load-time.
Line [16] currently accepts bare filenames (for example local.db), which breaks this repo’s configuration-path convention.
Suggested validation guard
impl RaindexProvider {
pub(crate) async fn load(
registry_url: &str,
db_path: Option<PathBuf>,
) -> Result<Self, RaindexProviderError> {
+ if let Some(path) = db_path.as_ref() {
+ let has_parent_dir = path
+ .parent()
+ .is_some_and(|parent| !parent.as_os_str().is_empty());
+ if !has_parent_dir {
+ return Err(RaindexProviderError::ClientInit(
+ "db_path must include a directory prefix (e.g., data/raindex.db)".into(),
+ ));
+ }
+ }
+
let url = registry_url.to_string();
let db = db_path.clone();Based on learnings: "In ST0x-Technology/st0x.rest.api, Rust source files that deal with configuration should specify database paths with a directory prefix ... never use bare filenames like local.db."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub(crate) async fn load( | |
| registry_url: &str, | |
| db_path: Option<PathBuf>, | |
| ) -> Result<Self, RaindexProviderError> { | |
| let url = registry_url.to_string(); | |
| let (tx, rx) = tokio::sync::oneshot::channel::<Result<DotrainRegistry, WorkerError>>(); | |
| let db = db_path.clone(); | |
| pub(crate) async fn load( | |
| registry_url: &str, | |
| db_path: Option<PathBuf>, | |
| ) -> Result<Self, RaindexProviderError> { | |
| if let Some(path) = db_path.as_ref() { | |
| let has_parent_dir = path | |
| .parent() | |
| .is_some_and(|parent| !parent.as_os_str().is_empty()); | |
| if !has_parent_dir { | |
| return Err(RaindexProviderError::ClientInit( | |
| "db_path must include a directory prefix (e.g., data/raindex.db)".into(), | |
| )); | |
| } | |
| } | |
| let url = registry_url.to_string(); | |
| let db = db_path.clone(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/raindex/config.rs` around lines 14 - 20, The load function should reject
bare filenames for db_path at load-time: in pub(crate) async fn load(...), if
let Some(db) = db_path.clone() { check db.parent() and ensure it is Some and not
empty (i.e., path has a directory prefix like "./data/local.db" or
"db/local.db"); if parent is None or empty, return an
Err(RaindexProviderError::InvalidConfig(...)) (or add a new RaindexProviderError
variant) with a clear message so consumers cannot pass bare "local.db".
Implement this validation early in load before using db.
Comment on lines
+34
to
+35
| let _raindex = shared_raindex.read().await; | ||
| todo!() |
There was a problem hiding this comment.
Replace todo!() in this handler before merge.
Line [35] unconditionally panics, so /v1/order/dca is effectively broken and bypasses the ApiError contract.
Immediate non-panicking fallback
- let _raindex = shared_raindex.read().await;
- todo!()
+ let _raindex = shared_raindex.read().await;
+ tracing::error!("DCA deploy handler is not implemented");
+ Err(ApiError::Internal(
+ "dca deployment is not implemented".into(),
+ ))As per coding guidelines: "All API errors must go through the ApiError enum, never return raw status codes."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let _raindex = shared_raindex.read().await; | |
| todo!() | |
| let _raindex = shared_raindex.read().await; | |
| tracing::error!("DCA deploy handler is not implemented"); | |
| Err(ApiError::Internal( | |
| "dca deployment is not implemented".into(), | |
| )) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/order/deploy_dca.rs` around lines 34 - 35, The handler currently
calls todo!() after acquiring _raindex (shared_raindex.read().await) which
panics and bypasses the ApiError contract; replace the todo!() with proper
non-panicking error handling that returns a Result with an ApiError variant
(e.g., Err(ApiError::InternalServerError or a more specific
ApiError::BadRequest) rather than panicking), ensure the function (deploy_dca
handler) returns the expected Result type, and propagate or map any errors from
shared_raindex access or subsequent logic into ApiError so all failures go
through the ApiError enum.
Comment on lines
+34
to
+35
| let _raindex = shared_raindex.read().await; | ||
| todo!() |
There was a problem hiding this comment.
todo!() in the live request path is a blocker.
Line [35] panics on every /v1/order/solver request and bypasses typed API error handling.
Immediate non-panicking fallback
- let _raindex = shared_raindex.read().await;
- todo!()
+ let _raindex = shared_raindex.read().await;
+ tracing::error!("Solver deploy handler is not implemented");
+ Err(ApiError::Internal(
+ "solver deployment is not implemented".into(),
+ ))As per coding guidelines: "All API errors must go through the ApiError enum, never return raw status codes."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let _raindex = shared_raindex.read().await; | |
| todo!() | |
| let _raindex = shared_raindex.read().await; | |
| tracing::error!("Solver deploy handler is not implemented"); | |
| Err(ApiError::Internal( | |
| "solver deployment is not implemented".into(), | |
| )) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/order/deploy_solver.rs` around lines 34 - 35, The handler
currently calls todo!() (after awaiting shared_raindex.read().await) which
panics on every /v1/order/solver request; replace the panic with a proper
ApiError return so the error flows through typed API error handling.
Specifically, remove todo!() and return an Err(ApiError::...) from the handler
(e.g., ApiError::Internal or a more specific variant like ServiceUnavailable)
with a clear message such as "solver endpoint not implemented" and include any
context you want from shared_raindex; ensure the function signature (the route
handler that awaited shared_raindex.read().await) still returns a Result so the
Err variant is used instead of panicking.
Comment on lines
+37
to
+38
| let _raindex = shared_raindex.read().await; | ||
| todo!() |
There was a problem hiding this comment.
todo!() in both orders handlers is a blocker.
Lines [38] and [73] panic on every request, so both endpoints are effectively down and no longer return typed API errors.
As per coding guidelines: "All API errors must go through the ApiError enum, never return raw status codes."
Also applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/orders.rs` around lines 37 - 38, The handlers currently call
todo!() after acquiring the read lock on shared_raindex (let _raindex =
shared_raindex.read().await) which panics and breaks the endpoints; replace
todo!() in both order handlers with real logic that uses the acquired read guard
(use the variable name e.g., raindex) to perform the lookup/operation and return
Result<..., ApiError>, mapping any failure to the appropriate ApiError variant
instead of panicking or returning raw status codes; ensure all error paths
convert errors into ApiError (via ? or map_err) and that the handler signatures
return the ApiError-based Response type so the endpoints return typed API errors
rather than panics.
Comment on lines
+37
to
+38
| let _raindex = shared_raindex.read().await; | ||
| todo!() |
There was a problem hiding this comment.
Both trades handlers panic unconditionally (todo!()), which is a release blocker.
Lines [38] and [73] make /v1/trades/tx/{tx_hash} and /v1/trades/{address} non-functional and bypass ApiError handling.
As per coding guidelines: "All API errors must go through the ApiError enum, never return raw status codes."
Also applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/trades.rs` around lines 37 - 38, The handlers for
/v1/trades/tx/{tx_hash} and /v1/trades/{address} currently call todo!() and thus
panic; replace the todo!() in the blocks that acquire
shared_raindex.read().await with real logic that queries the raindex and returns
results or an ApiError variant instead of panicking. Specifically, in the
function(s) handling tx_hash and address use the read guard from shared_raindex
to perform the lookup, map a missing result to the appropriate ApiError (e.g.,
ApiError::NotFound or the project's canonical error variant) and map any
internal errors to ApiError::Internal or ApiError::ServiceUnavailable as
appropriate, then return Ok(response) on success; ensure every failure path
returns Err(ApiError::...) so no raw status codes or panics remain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dependent PRs
Motivation
Every API request previously spawned a new OS thread + Tokio runtime to create a fresh
RaindexClientviarun_with_client(). This was necessary because the oldRaindexClientusedRc/RefCellinternally and wasn'tSend+Sync.The upstream rain.orderbook now makes
RaindexClientSend+Syncon native (usesArc<Mutex<...>>), andnew_native()starts the sync scheduler internally. This lets us create a single shared client at startup and pass it to all handlers via Rocket state.Solution
5c78970c25which provides Send+SyncRaindexClientRaindexProviderto hold a sharedRaindexClientcreated at startup. Thread spawning is kept only for construction (DotrainRegistry future is not Send due to JsValue)sync.rs— sync is now handled internally byRaindexClient::new_native()Send+Sync— changeasync_trait(?Send)toasync_traitonOrderDataSourceandSwapDataSourcerun_with_clientpattern with directraindex.client()accessNet result: -401 lines, +115 lines across 18 files. Local benchmarks show ~2s response times (quote/calldata), consistent with local DB performance.
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Refactor
Chores