Repository navigation
test(stress): add API capacity admin-user harness - #5855
Conversation
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Changesironclaw_stress api-user-capacity scenario
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: docs
![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 the api-user-capacity scenario to the ironclaw_stress tool, enabling end-to-end WebUI API load testing with support for provisioning test users via an admin API. It also enhances the mock LLM server to track and report detailed request metrics such as latencies, concurrency, and request spreads. Feedback on these changes highlights a critical issue in the wait_for_assistant polling loop, which currently lacks any delay between iterations, resulting in a tight busy-waiting loop that could overwhelm both server and client resources.
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 deadline = Instant::now() + Duration::from_millis(args.api_terminal_timeout_ms); | ||
| loop { | ||
| let timeline = harness.timeline(user).await; | ||
| let value = timeline.value.clone(); | ||
| api_samples.push(timeline.sample); | ||
| match value { | ||
| Ok(value) => { | ||
| if timeline_has_finalized_assistant(&value) { | ||
| return None; | ||
| let finalized_count = timeline_finalized_assistant_count(&value); | ||
| if finalized_count >= target_finalized_count { | ||
| return Ok(finalized_count); | ||
| } | ||
| } | ||
| Err(failure) => return Some(failure), | ||
| Err(failure) => return Err(failure), | ||
| } | ||
| if Instant::now() >= deadline { | ||
| return Some(FailureCause::new( | ||
| return Err(FailureCause::new( | ||
| "api_full_flow_timeout", | ||
| "timeline", | ||
| format!( |
There was a problem hiding this comment.
The polling loop in wait_for_assistant continuously calls harness.timeline(user).await without any delay/sleep between iterations. This creates a tight busy-waiting loop that can exhaust connection pools, consume excessive CPU on the client, and hammer the server with unnecessary requests under stress testing.
Consider adding a small delay (e.g., 100ms or 200ms) between polling attempts using tokio::time::sleep as a pragmatic tradeoff for waiting in non-critical paths.
let deadline = Instant::now() + Duration::from_millis(args.api_terminal_timeout_ms);
loop {
let timeline = harness.timeline(user).await;
let value = timeline.value.clone();
api_samples.push(timeline.sample);
match value {
Ok(value) => {
let finalized_count = timeline_finalized_assistant_count(&value);
if finalized_count >= target_finalized_count {
return Ok(finalized_count);
}
}
Err(failure) => return Err(failure),
}
if Instant::now() >= deadline {
return Err(FailureCause::new(
"api_full_flow_timeout",
"timeline",
format!(
"timed out waiting for assistant message count {target_finalized_count}",
),
));
}
tokio::time::sleep(Duration::from_millis(200)).await;
}References
- A fixed-duration sleep (e.g.,
tokio::time::sleep) can be a pragmatic tradeoff for waiting on resource initialization in non-critical paths, especially if a fallback mechanism (like logging) exists and a more robust solution is out of scope.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 2 | 0 | 2 | b634ecbdb93a |
Head: b634ecbdb93a789fef162ee5338ad62a140506de
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found blocking issues in the new api-user-capacity assistant wait accounting. Static review only; Rust tests could not be run because cargo is unavailable in this environment.
Findings
Blocking: 2 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Cumulative assistant count cannot work with paged timelines
Location: tools/ironclaw_stress/src/api_capacity.rs:668-669
The new wait logic compares the cumulative per-user target against the number of finalized assistant messages in a single timeline page. The timeline endpoint is bounded by --api-page-size (default 50, server max 200), while expected_finalized_assistant_count grows with every operation in the thread. With default --operations 200, the count returned here tops out at the current page's assistants, so later operations time out even though their assistant replies finalized. Track the submitted message/run, use pagination, or maintain the baseline from a sufficiently complete source instead of comparing a cumulative count to one page.
2. ❌ [MEDIUM] Failed waits advance the expected assistant count
Location: tools/ironclaw_stress/src/api_capacity.rs:611-612
On any wait failure this returns target_finalized_count, so the next operation assumes the missing assistant reply exists. If the timeout or error was caused by a real model/server failure and no assistant is ever finalized, all following operations wait for an unreachable count and get reported as failures too. Keep the previous observed count on failure, or return the last observed finalized count from wait_for_assistant.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/stale/stalled state while reviewers run.
| Ok(value) => { | ||
| if timeline_has_finalized_assistant(&value) { | ||
| return None; | ||
| let finalized_count = timeline_finalized_assistant_count(&value); |
There was a problem hiding this comment.
This compares a cumulative target to only the current timeline page. Since timeline responses are bounded by --api-page-size (default 50, server max 200), longer runs eventually time out despite successful assistant replies once older messages fall off the page. The wait needs to track the submitted message/run, page through history, or use a baseline from a complete count.
| stages: None, | ||
| }, | ||
| api_samples, | ||
| target_finalized_count, |
There was a problem hiding this comment.
Returning target_finalized_count on a wait failure advances state as if this assistant reply finalized. If the failure is real and no reply is ever added, every later operation waits for a count that cannot be reached and gets misreported as failed. Preserve the previous count or return the last observed count from the waiter.
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.12% — 283863 / 333481 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)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 222-236: The mutex handling in MockLlmState is silently swallowing
PoisonError in summary, record_request_start, and record_request_latency, which
can hide broken metrics. Update the MockLlmState methods so lock failures are
not defaulted or ignored: either propagate the mutex error from summary or log
it explicitly before returning, and make the
record_request_start/record_request_latency helpers emit a diagnostic when
locking fails instead of dropping the sample. Use the existing MockLlmState
method names to keep the change localized.
🪄 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: 9941add1-7979-4e41-8cb0-92f04a69b886
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (5)
tools/ironclaw_stress/Cargo.tomltools/ironclaw_stress/README.mdtools/ironclaw_stress/src/api_capacity.rstools/ironclaw_stress/src/main.rstools/ironclaw_stress/src/tests.rs
| let request_latencies = self | ||
| .request_latencies | ||
| .lock() | ||
| .map(|latencies| latencies.clone()) | ||
| .unwrap_or_default(); | ||
| let mut request_latency_us = request_latencies | ||
| .iter() | ||
| .map(Duration::as_micros) | ||
| .collect::<Vec<_>>(); | ||
| request_latency_us.sort_unstable(); | ||
| let request_start_offsets = self | ||
| .request_start_offsets | ||
| .lock() | ||
| .map(|offsets| offsets.clone()) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Silent-failure on mutex poisoning in MockLlmState.
summary() uses .unwrap_or_default() on Mutex::lock() results (lines 226, 236), and record_request_start/record_request_latency use if let Ok(...) (lines 262, 268). All three silently swallow PoisonError, producing empty metrics or dropped samples with no diagnostic. Per the "fail loud" invariant, errors should propagate or at minimum be logged — a poisoned mutex means the summary silently reports zero latencies/offsets.
🔧 Proposed fix: log on poison instead of silent default
let request_latencies = self
.request_latencies
.lock()
- .map(|latencies| latencies.clone())
- .unwrap_or_default();
+ .map(|latencies| latencies.clone())
+ .unwrap_or_else(|e| {
+ eprintln!("mock llm: request_latencies mutex poisoned: {e}");
+ Vec::new()
+ });
// ...
let request_start_offsets = self
.request_start_offsets
.lock()
- .map(|offsets| offsets.clone())
- .unwrap_or_default();
+ .map(|offsets| offsets.clone())
+ .unwrap_or_else(|e| {
+ eprintln!("mock llm: request_start_offsets mutex poisoned: {e}");
+ Vec::new()
+ });And for the record helpers:
fn record_request_start(&self, offset: Duration) {
- if let Ok(mut offsets) = self.request_start_offsets.lock() {
+ match self.request_start_offsets.lock() {
+ Ok(mut offsets) => offsets.push(offset),
+ Err(e) => eprintln!("mock llm: request_start_offsets mutex poisoned: {e}"),
}
}
fn record_request_latency(&self, latency: Duration) {
- if let Ok(mut latencies) = self.request_latencies.lock() {
+ match self.request_latencies.lock() {
+ Ok(mut latencies) => latencies.push(latency),
+ Err(e) => eprintln!("mock llm: request_latencies mutex poisoned: {e}"),
}
}Also applies to: 261-271
🤖 Prompt for 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.
In `@tools/ironclaw_stress/src/api_capacity.rs` around lines 222 - 236, The mutex
handling in MockLlmState is silently swallowing PoisonError in summary,
record_request_start, and record_request_latency, which can hide broken metrics.
Update the MockLlmState methods so lock failures are not defaulted or ignored:
either propagate the mutex error from summary or log it explicitly before
returning, and make the record_request_start/record_request_latency helpers emit
a diagnostic when locking fails instead of dropping the sample. Use the existing
MockLlmState method names to keep the change localized.
Source: Path instructions
|
🚅 Deployed to the ironclaw-pr-5855 environment in ironclaw-ci-preview
|
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | 1f697cc8fdeb |
Head: 1f697cc8fdeb4ba4e8ddd925b08079d15174304e
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found a blocking correctness issue in the API capacity harness: the new assistant-count wait logic can time out under the default workload once timeline pagination hides older messages.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Do not compare absolute assistant count to a single timeline page
Location: tools/ironclaw_stress/src/api_capacity.rs:696-698
target_finalized_count is accumulated over the whole thread, but harness.timeline() only requests the latest api_page_size messages. With the default page size of 50, each turn adds a user and assistant message, so after about 25 turns the current page can never contain enough finalized assistant messages to satisfy target_finalized_count; the default --operations 200 API capacity run will start timing out even when assistants are being produced. Page through history, raise the requested limit based on the target, or wait on a per-operation identifier/run instead of a whole-thread count from one page.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/failed/superseded state while reviewers run.
| if timeline_has_finalized_assistant(&value) { | ||
| return None; | ||
| let finalized_count = timeline_finalized_assistant_count(&value); | ||
| if finalized_count >= target_finalized_count { |
There was a problem hiding this comment.
This compares an absolute whole-thread assistant count to only the current timeline page. With the default api_page_size of 50, a user doing more than about 25 turns can never see finalized_count >= target_finalized_count, so the default --operations 200 run times out despite successful assistants. Please page through history, adjust the requested limit for the target, or wait on a per-operation/run identifier.
This branch was successfully deployed
1 participant
Summary
This is the harness-only base PR for the API capacity / chat latency work. It intentionally avoids the runtime performance changes so the measurement surface can be reviewed and merged independently.
Changes:
ironclaw_stress --scenario api-user-capacityto provision many real API users through WebUI admin CRUD.list_threads,timeline,session), and mock-LLM request timing stats.Benchmark Context
This PR is the measurement foundation. The current local baseline captured on the stacked runtime branch was:
c100/u100, 1 op/user, no read hammer: full-flow p951.85s, throughput53.0 ops/sec.c100/u100, 1 op/user, 2 read QPS/user: full-flow p951.97-1.99s, throughput49.6-50.8 ops/sec.list_threadsp95771-887ms,timelinep95835-876ms.100/100requests, p95~2.5-3ms, max in-flight5-6.Those runtime gains are not part of this PR; they will be stacked on top.
Timeline So Far
100 users / 100 concurrencyinstead of unrealistic single-user-only pressure.Verification
CARGO_TARGET_DIR=/Volumes/NVME/ironclaw-target-api-admin CARGO_INCREMENTAL=0 cargo test -p ironclaw_stress finalized_assistant -- --nocaptureCARGO_TARGET_DIR=/Volumes/NVME/ironclaw-target-api-admin CARGO_INCREMENTAL=0 cargo build -p ironclaw_stress