Conversation
MitchTurner
requested review from
a team,
Dentosal,
rymnc and
xgreenx
as code owners
PR SummaryMedium Risk Overview Wires the guard into the GraphQL schema builder and exposes new CLI/config knobs ( Expands integration tests to issue full-block GraphQL queries (via Written by Cursor Bugbot for commit bd06491. This will update automatically on new commits. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
crates/fuel-core/src/graphql_api/extensions/expensive_op_guard.rs
Outdated
Show resolved
Hide resolved
| #[clap(long = "full-block-request-timeout", default_value = "3s", env)] | ||
| pub full_block_request_timeout: humantime::Duration, |
There was a problem hiding this comment.
Might be nice if this was optional for local envs, but just setting a high limit should be fine
There was a problem hiding this comment.
I looked into this, it makes the code a bit more complicated. Still worth considering, but not prioritizing now.
| self.semaphore.available_permits(), | ||
| ); | ||
|
|
||
| match is_expensive { |
| let out = tokio::time::timeout(self.timeout, fut).await; | ||
| tracing::warn!( | ||
| "finished executing in {:?}ns, success: {:?}", | ||
| starting_time.elapsed().as_nanos(), |
There was a problem hiding this comment.
we could probably just use the Debug impl of Duration here, it gives nice units by default
|
Thanks for the contribution! Unfortunately we can't verify the commit author(s): Codex <c***@.local>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Fuel Labs Contributor License Agreement and this Pull Request will be revalidated. |
| "finished executing in {:?}ns, success: {:?}", | ||
| starting_time.elapsed().as_nanos(), | ||
| out.is_ok(), | ||
| ); |
There was a problem hiding this comment.
Warn-level logging on every expensive request execution
Medium Severity
tracing::warn! is used unconditionally for every expensive operation execution, including successful ones. In production with many legitimate block/blocks queries, this generates a warning-level log entry per request. Warning level is for conditions that may need attention — routine successful completions belong at debug! or info! level. This will create significant log noise and may mask real warnings.
| ) | ||
| }) | ||
| .sum() | ||
| } |
There was a problem hiding this comment.
Guard counts fields across all operations, not executed one
Medium Severity
expensive_root_field_count iterates over all operations in the document (doc.operations.iter()) and sums their expensive fields. In GraphQL, a document can contain multiple named operations but only one is executed per request. This over-counts permits needed, potentially rejecting legitimate requests that execute a cheap operation but whose document happens to also define expensive ones.
MitchTurner
force-pushed
the
feature/minimize-impact-of-expensive-block-calls
branch
from
f0c89d8 to
d97ac7e
Compare
MitchTurner
force-pushed
the
feature/minimize-impact-of-expensive-block-calls
branch
from
d97ac7e to
c57d35a
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| "finished executing in {:?}ns, success: {:?}", | ||
| starting_time.elapsed(), | ||
| out.is_ok(), | ||
| ); |
There was a problem hiding this comment.
Log message appends spurious "ns" after Duration debug format
Low Severity
The format string "finished executing in {:?}ns, success: {:?}" appends a literal ns after the Debug representation of a Duration, which already includes its own unit. This produces garbled output like "finished executing in 7msns" or "finished executing in 1.234sns". The ns suffix needs to be removed, or starting_time.elapsed().as_nanos() used instead.
3 participants
Linked Issues/PRs
Description
This is a protection against DoS by spamming expensive requests that can block the threadpool. We will be migrating to a better, paginated request pattern, but that will be a breaking change.
The plan for now is add this change as a stop-gap, depricate this endpoint and move users to the new pattern.
Checklist
Before requesting review