feat(stress): add API capacity workload - #5781
Conversation
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted review state before this projection. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds an ChangesAPI Capacity Stress Scenario
Estimated code review effort: 4 (Complex) | ~60 minutes User-Turn Stage Tracing and Blocking Governor Calls
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
github-actions
Bot
added
scope: dependencies
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new api-user-capacity stress testing scenario to the ironclaw_stress tool, adding the api_capacity module to simulate virtual users and background read workers, along with a built-in mock LLM sidecar. Key feedback highlights several critical robustness issues: potential division-by-zero panics if the user count is zero or if the mock LLM jitter is set to maximum; integer overflow and memory exhaustion risks with large HTTP content lengths; and a resource leak vulnerability when blocking resource governor tasks are cancelled. Additionally, the reviewer suggested using tokio::time::interval to accurately maintain target QPS under load and improving HTTP response handling to prevent network errors from being masked as JSON decoding failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let aggregate_qps = args.api_read_qps_per_user * args.users as f64; | ||
| let worker_count = read_worker_count(args).max(1) as f64; | ||
| let sleep = if aggregate_qps <= 0.0 { | ||
| Duration::from_secs(3600) | ||
| } else { | ||
| Duration::from_secs_f64((worker_count / aggregate_qps).max(0.001)) | ||
| }; | ||
| let mut api_samples = Vec::new(); | ||
| let mut operation_index = 0_u64; | ||
| loop { | ||
| tokio::select! { | ||
| _ = stop_reads.notified() => break, | ||
| _ = tokio::time::sleep(sleep) => {} | ||
| } |
There was a problem hiding this comment.
Sleeping for a constant duration in the read worker loop does not compensate for request latency, which will significantly throttle the achieved QPS under load. Additionally, if args.api_read_qps_per_user is extremely small, Duration::from_secs_f64 can panic on overflow. Using tokio::time::interval with MissedTickBehavior::Delay allows the loop to dynamically compensate for request latency and maintain the target QPS accurately.
let aggregate_qps = args.api_read_qps_per_user * args.users as f64;
let worker_count = read_worker_count(args).max(1) as f64;
let sleep = if aggregate_qps <= 0.0 {
Duration::from_secs(3600)
} else {
let sleep_secs = (worker_count / aggregate_qps).max(0.001);
if sleep_secs.is_nan() || sleep_secs.is_infinite() || sleep_secs > 3600.0 {
Duration::from_secs(3600)
} else {
Duration::from_secs_f64(sleep_secs)
}
};
let mut interval = tokio::time::interval(sleep);
interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay);
let mut api_samples = Vec::new();
let mut operation_index = 0_u64;
loop {
tokio::select! {
_ = stop_reads.notified() => break,
_ = interval.tick() => {}
}| let status = response.status(); | ||
| let text = response.text().await.unwrap_or_default(); | ||
| if !status.is_success() { | ||
| Err(FailureCause::new( | ||
| format!("api_http_status_{}", status.as_u16()), | ||
| name, | ||
| truncate_detail(format!("{url}: {text}")), | ||
| )) | ||
| } else { | ||
| serde_json::from_str::<Value>(&text).map_err(|error| { | ||
| FailureCause::new("api_json_decode", name, format!("{url}: {error}")) | ||
| }) | ||
| } |
There was a problem hiding this comment.
When validating or processing HTTP responses, check for successful status codes (e.g., 2xx) first to short-circuit and avoid the performance overhead of parsing the response body as JSON. Additionally, using unwrap_or_default() on response.text().await can hide actual network or timeout errors that occur during the body download, reporting them as JSON decode errors instead. It is better to check the status code first, and then match on the result of response.text().await to distinguish between a failure to retrieve the body and a failure to parse a successfully received body.
let status = response.status();
if !status.is_success() {
let text = response.text().await.unwrap_or_default();
return Err(FailureCause::new(
format!("api_http_status_{}", status.as_u16()),
name,
truncate_detail(format!("{url}: {text}")),
));
}
match response.text().await {
Ok(text) => {
serde_json::from_str::<Value>(&text).map_err(|error| {
FailureCause::new("api_json_decode", name, format!("{url}: {error}"))
})
}
Err(error) => Err(FailureCause::new("api_http_body", name, error)),
}References
- When validating or processing HTTP responses, check for successful status codes (e.g., 2xx) first to short-circuit and avoid the performance overhead of parsing the response body as JSON.
| users.sort_by_key(|user| user.index); | ||
| Ok(users) |
There was a problem hiding this comment.
If args.users is 0, setup_users will return an empty Vec<ApiUser> without error, which subsequently causes division-by-zero panics downstream (e.g., in run_read_worker). Adding a check to ensure at least one user is successfully set up prevents these panics.
if users.is_empty() {
return Err("No users were successfully set up. Ensure --users is greater than 0.".to_string());
}
users.sort_by_key(|user| user.index);
Ok(users)| let content_length = parse_content_length(&headers).unwrap_or(0); | ||
| while request.len() < header_end + 4 + content_length { | ||
| let read = stream | ||
| .read(&mut buffer) | ||
| .await | ||
| .map_err(|error| error.to_string())?; | ||
| if read == 0 { | ||
| break; | ||
| } | ||
| request.extend_from_slice(&buffer[..read]); | ||
| } |
There was a problem hiding this comment.
If content_length is extremely large, header_end + 4 + content_length can overflow usize (causing a panic in debug mode), and the loop can read unbounded amounts of data into memory (causing out-of-memory exhaustion). Enforcing a reasonable maximum limit on content_length prevents these issues.
let content_length = parse_content_length(&headers).unwrap_or(0);
if content_length > 10 * 1024 * 1024 {
write_mock_response(&mut stream, 413, "text/plain", b"payload too large").await?;
return Ok(());
}
while request.len() < header_end + 4 + content_length {
let read = stream
.read(&mut buffer)
.await
.map_err(|error| error.to_string())?;
if read == 0 {
break;
}
request.extend_from_slice(&buffer[..read]);
}| fn deterministic_jitter_ms(request_index: u64, jitter_ms: u64) -> u64 { | ||
| request_index | ||
| .wrapping_mul(1_103_515_245) | ||
| .wrapping_add(12_345) | ||
| % (jitter_ms + 1) | ||
| } |
There was a problem hiding this comment.
If jitter_ms is u64::MAX, jitter_ms + 1 will overflow to 0, causing a division-by-zero panic during the modulo operation. Using checked_add or saturating_add prevents this panic.
fn deterministic_jitter_ms(request_index: u64, jitter_ms: u64) -> u64 {
let divisor = jitter_ms.checked_add(1).unwrap_or(u64::MAX);
(request_index
.wrapping_mul(1_103_515_245)
.wrapping_add(12_345))
% divisor
}| async fn reserve_resources( | ||
| governor: Arc<dyn ResourceGovernor>, | ||
| scope: ResourceScope, | ||
| ) -> Result<ResourceReservation, OperationFailure> { | ||
| governor | ||
| .reserve(scope, resource_ops::estimate()) | ||
| tokio::task::spawn_blocking(move || governor.reserve(scope, resource_ops::estimate())) | ||
| .await | ||
| .map_err(|error| { | ||
| OperationFailure::new( | ||
| "resource_worker_join", | ||
| "resource_reserve", | ||
| format!("resource reserve worker failed: {error}"), | ||
| ) | ||
| })? | ||
| .map_err(|error| resource_failure("resource_reserve", error)) | ||
| } |
There was a problem hiding this comment.
When offloading synchronous resource governor calls to tokio::task::spawn_blocking, if the outer async future is cancelled (e.g., due to a timeout or task abort), the JoinHandle is dropped but the blocking task continues to run in the background. Once it completes, the returned ResourceReservation is dropped without being reconciled or released, leading to a permanent resource leak in the governor. Consider using a guard or registering the reservation ID in a way that ensures cleanup even if the outer future is cancelled.
|
🚅 Deployed to the ironclaw-pr-5781 environment in ironclaw-ci-preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/ironclaw_stress/src/api_capacity.rs`:
- Line 597: The body-read in the response handling path is swallowing transport
errors by converting them to an empty string. Update the response processing
logic around the `response.text().await` call in `api_capacity.rs` to propagate
the read failure as its own `FailureCause` instead of defaulting silently. Keep
the failure distinct from `api_json_decode` so the harness records the correct
cause, and apply the same fail-loud pattern anywhere else in the same
response-to-text flow.
- Around line 454-492: In run_read_worker, the stop signal handling should not
rely solely on Notify::notified() because a worker that is busy in an API call
can miss notify_waiters() and loop forever; switch to a shutdown mechanism that
is remembered across waits (for example, check a shared atomic/flag before each
iteration and after each request, or use a cancellation primitive that
persists). Also, when collecting API samples from the harness calls in
run_read_worker, do not swallow response body-read failures with
unwrap_or_default; propagate the error so TaskResult only records valid samples
and failures are visible to the caller.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 3c77664a-fb9a-44cd-8d98-7ad458ff6e97
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (8)
tools/ironclaw_stress/Cargo.tomltools/ironclaw_stress/src/api_capacity.rstools/ironclaw_stress/src/human.rstools/ironclaw_stress/src/main.rstools/ironclaw_stress/src/process_pressure.rstools/ironclaw_stress/src/resource_ops.rstools/ironclaw_stress/src/tests.rstools/ironclaw_stress/src/user_turn.rs
|
Addressed the review comments in Fixed Review Feedback
Validation
GitHub checks: pending after push. PR remains merge-conflicting against |
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.16% — 282152 / 331314 lines Per-crate breakdown (65 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (4 entry/entries excluded from the accounting above)
|
This branch was successfully deployed
1 participant
Summary
api-user-capacityscenario toironclaw_stressfor full API user flows plus concurrent read pressureStacking
Stacked on #5726 (
codex/hst-postgres-v2-03-runtime-stores). This PR is harness-only; storage/runtime optimizations should follow after this lands or while stacked above it.Verification
CARGO_TARGET_DIR=/Volumes/NVME/ironclaw-target-96d3 cargo test -p ironclaw_stressgit diff --checkCARGO_TARGET_DIR=/Volumes/NVME/ironclaw-target-96d3 cargo build -p ironclaw_stress --releaseNotes
This is intentionally split before further hillclimbing so future benchmark and optimization PRs can cite a stable workload shape.