Propagate subgraph errors instead of silently swallowing them#2494
Merged
Propagate subgraph errors instead of silently swallowing them#2494
Conversation
4 tasks
Collaborator
Author
|
After this PR merges we need to add this to the list of propagated errors. |
Contributor
📝 Walkthrough🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/subgraph/src/multi_orderbook_client.rs (1)
58-70: Factor the partial-success fold into one helper.The same
all_items + last_errorreduction now lives in three methods, but onlyorders_listhas the full new 429-specific regression coverage. A small shared helper and shared test matrix would make future behavior changes much less likely to drift between orders, vaults, and tokens.Based on learnings, "if error propagation behavior should change, implement the change consistently across all related methods in a single PR."
Also applies to: 127-139, 165-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/subgraph/src/multi_orderbook_client.rs` around lines 58 - 70, Factor the repeated reduction into a single helper (e.g., merge_partial_results or collect_partial_items) that is generic over item type and error type and reproduces the exact behavior: iterate results, extend a single Vec with all Ok(items), track the most recent Err as last_error, and if the final Vec is empty return Err(last_error) else return Ok(all_items). Replace the duplicated fold in orders_list and the other two methods (the other blocks at the ranges referenced) to call this helper; ensure the helper's signature fits the methods (consume the iterator of Result<Vec<T>, E> and return Result<Vec<T>, E>) and update tests to call the shared test matrix so the 429-specific behavior remains covered consistently across all three call sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/common/src/raindex_client/vaults.rs`:
- Line 1491: The subgraph call currently uses the fall-through "?.await" which
lets subgraph errors override successful local-db results in get_vaults() and
get_all_vault_tokens(); change the logic so that when
is_chain_supported_local_db() is true and local DB produced results, those
results are returned without being dropped by a subsequent subgraph error
(remove the direct ?.await propagation on the subgraph call, capture the
subgraph Result, and only propagate an error if all sources are empty), and add
a regression test exercising a mixed local_db + subgraph scenario where the
subgraph is rate-limited/down but local DB has items to ensure local results are
preserved.
---
Nitpick comments:
In `@crates/subgraph/src/multi_orderbook_client.rs`:
- Around line 58-70: Factor the repeated reduction into a single helper (e.g.,
merge_partial_results or collect_partial_items) that is generic over item type
and error type and reproduces the exact behavior: iterate results, extend a
single Vec with all Ok(items), track the most recent Err as last_error, and if
the final Vec is empty return Err(last_error) else return Ok(all_items). Replace
the duplicated fold in orders_list and the other two methods (the other blocks
at the ranges referenced) to call this helper; ensure the helper's signature
fits the methods (consume the iterator of Result<Vec<T>, E> and return
Result<Vec<T>, E>) and update tests to call the shared test matrix so the
429-specific behavior remains covered consistently across all three call sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3e9477b-1039-4dc1-bf74-56949fc6fbd5
📒 Files selected for processing (5)
crates/common/src/raindex_client/orders.rscrates/common/src/raindex_client/vaults.rscrates/subgraph/src/cynic_client.rscrates/subgraph/src/multi_orderbook_client.rscrates/subgraph/src/vault_balance_changes_query.rs
hardyjosh
approved these changes
Apr 10, 2026
JuaniRios
approved these changes
Apr 10, 2026
Collaborator
Author
Merge activity
|
Add HTTP status checking to CynicClient, change multi-orderbook client methods to return Result, and propagate errors to callers. When some subgraphs succeed and others fail, return partial results; only error when all subgraphs fail. Includes tests for rate-limiting (HTTP 429).
findolor
force-pushed
the
2026-03-02-subgraph-error-propagation-v1
branch
from
fcf90e6 to
a59df5b
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/subgraph/src/multi_orderbook_client.rs (1)
144-180:⚠️ Potential issue | 🟡 MinorAdd
tokens_listtests for new error semantics.
tokens_listnow has propagation logic identical to orders/vaults, but this file doesn’t include equivalent tests for all-error and partial-success paths. Please add parity tests to lock behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/subgraph/src/multi_orderbook_client.rs` around lines 144 - 180, Add unit tests covering the new error semantics in tokens_list: create two tests mirroring the orders/vaults cases — one where all subgraph clients return errors and tokens_list returns Err (propagating the last error from get_orderbook_subgraph_client/tokens_list_all), and one partial-success case where some subgraphs return tokens and others return errors and tokens_list returns Ok with the combined SgErc20WithSubgraphName items; locate the test helpers/mocks used by existing orders/vaults tests and reuse the same mocking for get_orderbook_subgraph_client and tokens_list_all, assert returned Err for the all-error test and assert combined Vec contents (including subgraph_name) for the partial-success test.
🧹 Nitpick comments (1)
crates/subgraph/src/multi_orderbook_client.rs (1)
58-70: Extract duplicated aggregation block into a shared helper.The same
all_* + last_errormerge logic is repeated three times. A generic helper would reduce drift risk and keep semantics aligned.♻️ Proposed refactor
+ fn merge_subgraph_results<T>( + results: Vec<Result<Vec<T>, OrderbookSubgraphClientError>>, + ) -> Result<Vec<T>, OrderbookSubgraphClientError> { + let mut all_items = Vec::new(); + let mut last_error = None; + for result in results { + match result { + Ok(items) => all_items.extend(items), + Err(e) => last_error = Some(e), + } + } + if all_items.is_empty() { + if let Some(e) = last_error { + return Err(e); + } + } + Ok(all_items) + }- let mut all_orders = Vec::new(); - let mut last_error = None; - for result in results { - match result { - Ok(items) => all_orders.extend(items), - Err(e) => last_error = Some(e), - } - } - if all_orders.is_empty() { - if let Some(e) = last_error { - return Err(e); - } - } + let mut all_orders = Self::merge_subgraph_results(results)?;Also applies to: 127-139, 165-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/subgraph/src/multi_orderbook_client.rs` around lines 58 - 70, Extract the repeated aggregation logic that collects Ok(Vec<T>) into a single Vec and returns the last Err if no items were collected into a generic helper like merge_results<T, E>(results: impl IntoIterator<Item = Result<Vec<T>, E>>) -> Result<Vec<T>, E>; replace the three in-place loops that use variables all_orders / all_* , last_error and iterate over results with calls to this helper (keeping the same semantics: extend on Ok, track last Err, and return Err only if the final collected Vec is empty and an Err was seen). Update call sites to import/use the helper and ensure its signature matches the error type used in those callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/subgraph/src/multi_orderbook_client.rs`:
- Around line 144-180: Add unit tests covering the new error semantics in
tokens_list: create two tests mirroring the orders/vaults cases — one where all
subgraph clients return errors and tokens_list returns Err (propagating the last
error from get_orderbook_subgraph_client/tokens_list_all), and one
partial-success case where some subgraphs return tokens and others return errors
and tokens_list returns Ok with the combined SgErc20WithSubgraphName items;
locate the test helpers/mocks used by existing orders/vaults tests and reuse the
same mocking for get_orderbook_subgraph_client and tokens_list_all, assert
returned Err for the all-error test and assert combined Vec contents (including
subgraph_name) for the partial-success test.
---
Nitpick comments:
In `@crates/subgraph/src/multi_orderbook_client.rs`:
- Around line 58-70: Extract the repeated aggregation logic that collects
Ok(Vec<T>) into a single Vec and returns the last Err if no items were collected
into a generic helper like merge_results<T, E>(results: impl IntoIterator<Item =
Result<Vec<T>, E>>) -> Result<Vec<T>, E>; replace the three in-place loops that
use variables all_orders / all_* , last_error and iterate over results with
calls to this helper (keeping the same semantics: extend on Ok, track last Err,
and return Err only if the final collected Vec is empty and an Err was seen).
Update call sites to import/use the helper and ensure its signature matches the
error type used in those callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a65841d9-d6d1-42a8-a5d0-1d94f6f0d488
📒 Files selected for processing (5)
crates/common/src/raindex_client/orders.rscrates/common/src/raindex_client/vaults.rscrates/subgraph/src/cynic_client.rscrates/subgraph/src/multi_orderbook_client.rscrates/subgraph/src/vault_balance_changes_query.rs
✅ Files skipped from review due to trivial changes (1)
- crates/common/src/raindex_client/orders.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/subgraph/src/vault_balance_changes_query.rs
- crates/subgraph/src/cynic_client.rs
- crates/common/src/raindex_client/vaults.rs
Contributor
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. SIZE=M |
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
3 participants
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.
Motivation
The
MultiOrderbookSubgraphClientmethods (orders_list,vaults_list,tokens_list) were silentlydiscarding subgraph errors via
filter_map(Result::ok). This meant HTTP errors (e.g. 429 rate limits,500s) were invisible to callers — queries would return empty results with no indication of failure.
Solution
orders_list,vaults_list, andtokens_listto returnResulttypes, propagatingOrderbookSubgraphClientErrorto callers.succeeds with results, the error is tolerated and partial results are returned.
CynicClientError::HttpErrorvariant to capture non-2xx HTTP responses with status code and body.orders.rsandvaults.rsto propagate errors with?.Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Bug Fixes
Tests