Add live latency trace instrumentation - #5472
Conversation
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new ChangesLive latency tracing rollout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 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 a new low-level observability crate, ironclaw_observability, and integrates live latency tracing across various core subsystems, including the filesystem, host runtime, process executor, turn scheduler, model gateway, and reborn runtime. The review feedback highlights a critical performance concern: unconditionally calling Instant::now() on hot paths introduces unnecessary timing overhead when latency tracing is disabled. To optimize this, the reviewer suggests lazily initializing timestamps as Option<Instant> using live_latency_enabled().then(Instant::now) and updating the tracing helpers to accept optional timestamps.
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.
| fn trace_fs_latency<T, E>( | ||
| operation: &'static str, | ||
| path: &ScopedPath, | ||
| started_at: Instant, | ||
| result: &Result<T, E>, | ||
| bytes: Option<usize>, | ||
| ) where | ||
| E: fmt::Display, | ||
| { | ||
| if !live_latency_enabled() { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Calling Instant::now() unconditionally on every filesystem operation introduces unnecessary overhead (system calls or VDSO reads) on hot paths when live latency tracing is disabled.
By changing started_at to Option<Instant> and initializing it with live_latency_enabled().then(Instant::now), we can completely avoid the timing overhead in the default production state.
| fn trace_fs_latency<T, E>( | |
| operation: &'static str, | |
| path: &ScopedPath, | |
| started_at: Instant, | |
| result: &Result<T, E>, | |
| bytes: Option<usize>, | |
| ) where | |
| E: fmt::Display, | |
| { | |
| if !live_latency_enabled() { | |
| return; | |
| } | |
| fn trace_fs_latency<T, E>( | |
| operation: &'static str, | |
| path: &ScopedPath, | |
| started_at: Option<Instant>, | |
| result: &Result<T, E>, | |
| bytes: Option<usize>, | |
| ) where | |
| E: fmt::Display, | |
| { | |
| let Some(started_at) = started_at else { | |
| return; | |
| }; |
| let started_at = Instant::now(); | ||
| let bytes = entry.body.len(); | ||
| let virtual_path = | ||
| self.resolve_with_permission(scope, path, FilesystemOperation::WriteFile)?; | ||
| self.root.put(&virtual_path, entry, cas).await | ||
| let result = self.root.put(&virtual_path, entry, cas).await; | ||
| trace_fs_latency("put", path, started_at, &result, Some(bytes)); |
There was a problem hiding this comment.
Avoid calling Instant::now() unconditionally when live latency tracing is disabled.
| let started_at = Instant::now(); | |
| let bytes = entry.body.len(); | |
| let virtual_path = | |
| self.resolve_with_permission(scope, path, FilesystemOperation::WriteFile)?; | |
| self.root.put(&virtual_path, entry, cas).await | |
| let result = self.root.put(&virtual_path, entry, cas).await; | |
| trace_fs_latency("put", path, started_at, &result, Some(bytes)); | |
| let started_at = live_latency_enabled().then(Instant::now); | |
| let bytes = entry.body.len(); | |
| let virtual_path = | |
| self.resolve_with_permission(scope, path, FilesystemOperation::WriteFile)?; | |
| let result = self.root.put(&virtual_path, entry, cas).await; | |
| trace_fs_latency("put", path, started_at, &result, Some(bytes)); |
| fn trace_runtime_latency_ok( | ||
| operation: &'static str, | ||
| thread_id: &ThreadId, | ||
| run_id: Option<TurnRunId>, | ||
| started_at: Instant, | ||
| ) { | ||
| if !live_latency_enabled() { | ||
| return; | ||
| } |
There was a problem hiding this comment.
By changing started_at to Option<Instant>, we can avoid calling Instant::now() when live latency tracing is disabled.
fn trace_runtime_latency_ok(
operation: &'static str,
thread_id: &ThreadId,
run_id: Option<TurnRunId>,
started_at: Option<Instant>,
) {
let Some(started_at) = started_at else {
return;
};| fn trace_runtime_latency_error<E>( | ||
| operation: &'static str, | ||
| thread_id: &ThreadId, | ||
| run_id: Option<TurnRunId>, | ||
| started_at: Instant, | ||
| error: &E, | ||
| ) where | ||
| E: fmt::Display + ?Sized, | ||
| { | ||
| if !live_latency_enabled() { | ||
| return; | ||
| } |
There was a problem hiding this comment.
By changing started_at to Option<Instant>, we can avoid calling Instant::now() when live latency tracing is disabled.
fn trace_runtime_latency_error<E>(
operation: &'static str,
thread_id: &ThreadId,
run_id: Option<TurnRunId>,
started_at: Option<Instant>,
error: &E,
) where
E: fmt::Display + ?Sized,
{
let Some(started_at) = started_at else {
return;
};| let total_started_at = Instant::now(); | ||
| let submit_started_at = Instant::now(); |
There was a problem hiding this comment.
In send_user_message, calling Instant::now() twice consecutively is redundant and adds unnecessary overhead. Furthermore, calling it unconditionally introduces timing overhead even when latency tracing is disabled.
We can optimize this by checking live_latency_enabled() first, reusing the timestamp, and passing Option<Instant> to the trace helpers.
let total_started_at = live_latency_enabled().then(Instant::now);
let submit_started_at = total_started_at;| fn trace_coordinator_latency_ok( | ||
| operation: &'static str, | ||
| scope: &TurnScope, | ||
| run_id: Option<TurnRunId>, | ||
| started_at: Instant, | ||
| ) { | ||
| if !live_latency_enabled() { | ||
| return; | ||
| } |
There was a problem hiding this comment.
By changing started_at to Option<Instant>, we can avoid calling Instant::now() when live latency tracing is disabled.
| fn trace_coordinator_latency_ok( | |
| operation: &'static str, | |
| scope: &TurnScope, | |
| run_id: Option<TurnRunId>, | |
| started_at: Instant, | |
| ) { | |
| if !live_latency_enabled() { | |
| return; | |
| } | |
| fn trace_coordinator_latency_ok( | |
| operation: &'static str, | |
| scope: &TurnScope, | |
| run_id: Option<TurnRunId>, | |
| started_at: Option<Instant>, | |
| ) { | |
| let Some(started_at) = started_at else { | |
| return; | |
| }; |
|
🚅 Deployed to the ironclaw-pr-5472 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_host_runtime/src/services/process_executor.rs (1)
172-210: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTrace the
dispatch_jsonfailure branch before?returns.Line 195 still exits on dispatcher failure before
runtime_dispatch_executeemits an error event, so the stage that most often stalls/fails stays invisible in the new latency target. Capture the mappedProcessExecutionError, trace it, then return it.As per path instructions, "Fail loud: flag silent-failure patterns ... Errors propagate with ? into thiserror types with context."
Possible fix
- let result = self - .dispatcher - .dispatch_json(CapabilityDispatchRequest { - capability_id: request.capability_id, - scope: request.scope, - estimate: request.estimate, - mounts: Some(request.mounts), - resource_reservation: request.resource_reservation, - input: request.input, - }) - .await - .map_err(|error| ProcessExecutionError::new(error.event_kind()))?; + let result = match self + .dispatcher + .dispatch_json(CapabilityDispatchRequest { + capability_id: request.capability_id, + scope: request.scope, + estimate: request.estimate, + mounts: Some(request.mounts), + resource_reservation: request.resource_reservation, + input: request.input, + }) + .await + { + Ok(result) => result, + Err(error) => { + let error = ProcessExecutionError::new(error.event_kind()); + trace_process_latency_error( + "runtime_dispatch_execute", + fields.as_ref(), + started_at, + &error, + ); + return Err(error); + } + };🤖 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 `@crates/ironclaw_host_runtime/src/services/process_executor.rs` around lines 172 - 210, The dispatch_json failure path in process_executor::runtime_dispatch_execute currently maps the error with ? before any latency/error trace is emitted, so the failure stays silent. Capture the mapped ProcessExecutionError from dispatcher.dispatch_json(), call trace_process_latency_error with "runtime_dispatch_execute", fields.as_ref(), and started_at, then return the error instead of using ?. Keep the existing cancellation and success tracing paths unchanged.Source: Path instructions
🤖 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 `@crates/ironclaw_filesystem/Cargo.toml`:
- Line 23: The ironclaw_observability dependency is declared inconsistently
across crates, with ironclaw_filesystem adding a version alongside path while
other dependents do not. Update the dependency entries in ironclaw_filesystem
and align them with the chosen workspace-wide convention used by
ironclaw_host_runtime and ironclaw_reborn_composition, keeping the declaration
consistent for ironclaw_observability so future version bumps or crates.io
publishing do not diverge.
In `@crates/ironclaw_filesystem/src/scoped.rs`:
- Around line 67-103: The `trace_fs_latency` error branch is leaking redacted
path/backend details by tracing `error = %error`, since `FilesystemError` can
format full `VirtualPath` and backend `reason` text. Update `trace_fs_latency`
to emit only a coarse error kind/variant label instead of the full error,
ideally by matching on `FilesystemError` (or adding a small mapper) and tracing
that sanitized value in the `live_latency_trace!` call. Keep the `path_class`
redaction intact and ensure failed filesystem ops never log raw paths or backend
internals.
- Around line 54-65: The scoped_path_class helper currently derives its label
from the first path segment, which can leak raw host aliases or tenant/user
identifiers into ironclaw_latency. Update scoped_path_class to return a fixed
bucket/category instead of echoing the segment from ScopedPath, and keep the
calling path in MountView::scoped_path or related logging code unchanged except
for using the redacted class label.
In `@crates/ironclaw_host_runtime/src/production.rs`:
- Around line 77-103: `trace_capability_latency_error` is logging the raw
`%error` into `ironclaw_latency`, which can leak backend or user content. Update
this helper to emit a fixed sanitized failure kind/category field instead of
formatting the error directly, and keep the existing metadata from `operation`,
`capability_id`, and `scope` so callers of `trace_capability_latency_error`
still get latency context without exposing the underlying error string.
In `@crates/ironclaw_host_runtime/src/services/process_executor.rs`:
- Around line 10-34: Process latency events are currently dropping parts of the
request scope, so update ProcessLatencyFields::from_request to carry the full
ResourceScope information from request.scope instead of only
thread_id/invocation_id. Add the missing tenant/user and any other scoped
identifiers used elsewhere in the runtime, and keep the field extraction aligned
with how production.rs reads ResourceScope so process latency records preserve
the same authority/state/event scope invariants.
In `@crates/ironclaw_host_runtime/src/turn_scheduler/latency.rs`:
- Around line 11-28: The latency helpers are collapsing canonical scope into
only thread_id, which drops tenant/agent/project context from scheduler traces.
Update run_fields_from_wake and scope_thread_id to carry the full scoped
identity from TurnRunWake/TurnScope instead of formatting just thread_id, and
make sure the callers in the scheduler latency path keep using that canonical
scope for notify_queued_run, claim_next_run*, and execute_claimed_run events.
In `@crates/ironclaw_observability/src/lib.rs`:
- Around line 25-30: The live_latency_trace! macro is expanding to
tracing::trace! at the call site, which creates an unwanted downstream
dependency on tracing being in scope. Update the macro in lib.rs to invoke the
tracing macro through $crate::tracing::trace! and make sure the crate re-exports
tracing so callers can use live_latency_trace! without importing tracing
directly.
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 119-166: The new tracing helpers in runtime.rs duplicate
boilerplate and push an already oversized file further past the “shrink when
touched” guideline. Move trace_runtime_latency_ok and
trace_runtime_latency_error, along with any runtime-composition-specific tracing
glue, into a dedicated module/file and have runtime.rs call into that shared
location, following the existing patterns used by model_gateway.rs,
turn_run_executor.rs, and coordinator.rs.
In `@crates/ironclaw_reborn/Cargo.toml`:
- Line 59: Align the `ironclaw_observability` dependency declaration in
`Cargo.toml` with the other workspace crates by removing the unnecessary pinned
version from the path dependency. Update the `ironclaw_observability` entry so
it matches the style used by `ironclaw_host_runtime` and
`ironclaw_reborn_composition`, keeping the path-based dependency consistent
across the workspace.
In `@crates/ironclaw_reborn/src/model_gateway.rs`:
- Around line 66-113: The new trace wrapper helpers in trace_model_latency_ok
and trace_model_latency_error are duplicated across multiple crates and are
making this already-large file grow further; centralize the shared ok/error
latency-tracing shape into ironclaw_observability instead of keeping per-crate
copies. Add a generic helper that accepts the component name plus the shared
identifiers and started_at/error data, then update model_gateway.rs and the
matching helpers in turn_run_executor.rs, runtime.rs, and coordinator.rs to call
it with only their component-specific values.
---
Outside diff comments:
In `@crates/ironclaw_host_runtime/src/services/process_executor.rs`:
- Around line 172-210: The dispatch_json failure path in
process_executor::runtime_dispatch_execute currently maps the error with ?
before any latency/error trace is emitted, so the failure stays silent. Capture
the mapped ProcessExecutionError from dispatcher.dispatch_json(), call
trace_process_latency_error with "runtime_dispatch_execute", fields.as_ref(),
and started_at, then return the error instead of using ?. Keep the existing
cancellation and success tracing paths unchanged.
🪄 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: 839871e4-8b9f-41ae-a3a6-ef57e03edad0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (19)
Cargo.tomlcrates/ironclaw_filesystem/Cargo.tomlcrates/ironclaw_filesystem/src/scoped.rscrates/ironclaw_host_runtime/Cargo.tomlcrates/ironclaw_host_runtime/src/production.rscrates/ironclaw_host_runtime/src/services/process_executor.rscrates/ironclaw_host_runtime/src/turn_scheduler.rscrates/ironclaw_host_runtime/src/turn_scheduler/executor_task.rscrates/ironclaw_host_runtime/src/turn_scheduler/latency.rscrates/ironclaw_observability/Cargo.tomlcrates/ironclaw_observability/src/lib.rscrates/ironclaw_reborn/Cargo.tomlcrates/ironclaw_reborn/src/model_gateway.rscrates/ironclaw_reborn/src/turn_run_executor.rscrates/ironclaw_reborn_composition/Cargo.tomlcrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_turns/Cargo.tomlcrates/ironclaw_turns/src/coordinator.rsdocs/internal/live-latency-instrumentation.md
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/ironclaw_host_runtime/src/production.rs (1)
440-471: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEarly
invoke_capabilityrejections never emit the overall latency trace.Policy-rejected (line 446) and trust-rejected (line 463) exits only emit the stage trace (
invoke_capability_policy_rejected/invoke_capability_trust_rejected), not aninvoke_capabilitytrace againsttotal_started_at. Compare with theauth_requiredexit at lines 503-508, which fires both the stage trace and aninvoke_capabilityok trace. Since this PR's stated goal is correlating stalls that "resolve together," dropping the top-level metric on two of the early-exit paths leaves gaps in theinvoke_capabilitytimeline for exactly the fast-fail cases that matter for diagnosing stuck-vs-rejected requests.🔧 Proposed fix
trace_capability_latency_ok( "invoke_capability_policy_rejected", &capability_id, &scope, total_started_at, ); + trace_capability_latency_ok( + "invoke_capability", + &capability_id, + &scope, + total_started_at, + ); return Ok(runtime_policy_failure(capability_id, error));(same pattern for the trust-rejected branch)
🤖 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 `@crates/ironclaw_host_runtime/src/production.rs` around lines 440 - 471, Early policy- and trust-rejected exits in invoke_capability only record the stage-specific trace and skip the top-level invoke_capability latency trace. Update the early-return branches in invoke_capability around enforce_runtime_policy and evaluate_invocation_trust to also emit the overall trace using total_started_at, matching the auth_required path and preserving a complete latency timeline for fast-fail cases.crates/ironclaw_reborn_composition/src/runtime/latency.rs (1)
1-41: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
reborn_runtimelatency events drop tenant/agent/project/mission scope.Unlike the peer helpers in
production.rs(host_runtime),process_executor.rs(process_executor), and mostlycoordinator.rs(turn_coordinator), these functions only carrythread_idandrun_id— notenant_id,agent_id,project_id, ormission_id. Callers inruntime.rs(e.g.submit_user_turn) already compute a fullscopeviaturn_scope_for, so the data is available but not threaded through. This breaks cross-component correlation for exactly the multi-tenant stall-diagnosis use case this PR is for, and runs counter to the explicit invariant.As per coding guidelines, "Preserve tenant/user/agent/project/mission/thread scope on authority, state, memory, process, network, outbound, resource, and event records."
🔧 Proposed fix
+use ironclaw_turns::TurnScope; + pub(super) fn trace_runtime_latency_ok( operation: &'static str, - thread_id: &ThreadId, + scope: &TurnScope, run_id: Option<TurnRunId>, started_at: Option<Instant>, ) { let run_id = run_id.map(|id| id.to_string()).unwrap_or_default(); ironclaw_observability::live_latency_trace_ok!( "reborn_runtime", operation, started_at, - thread_id = %thread_id, + tenant_id = %scope.tenant_id, + agent_id = scope.agent_id.as_ref().map(|id| id.as_str()).unwrap_or(""), + project_id = scope.project_id.as_ref().map(|id| id.as_str()).unwrap_or(""), + thread_id = %scope.thread_id, run_id = run_id.as_str(), "reborn runtime operation completed", ); }(mirror for the error helper; call sites that don't yet have
scoperesolved would need it threaded through earlier, e.g. beforesubmit_user_turncallsaccept_inbound_message)🤖 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 `@crates/ironclaw_reborn_composition/src/runtime/latency.rs` around lines 1 - 41, The reborn_runtime latency helpers are only tagging thread_id and run_id, so they lose tenant/agent/project/mission scope needed for correlation. Update trace_runtime_latency_ok and trace_runtime_latency_error to accept the full scope data and pass those fields into the live_latency_trace_ok!/live_latency_trace_error! macros, matching the patterns used in production.rs, process_executor.rs, and coordinator.rs. Then thread the already-computed scope from runtime.rs (for example from submit_user_turn via turn_scope_for) through the call sites so both helpers emit the complete scope.Source: Coding guidelines
🤖 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 `@crates/ironclaw_filesystem/src/scoped.rs`:
- Around line 54-56: The current scoped_path_class implementation in scoped.rs
ignores the ScopedPath input and returns a constant, which makes all trace
events lose useful categorization. Update scoped_path_class to inspect the fixed
top-level mount segment from ScopedPath and map it into a small set of redacted
buckets (for example workspace, memory, artifacts, or other) so put/get/query
traces still emit a meaningful path_class without exposing raw identifiers. Use
the existing scoped_path_class and ScopedPath symbols to keep the change
localized, and preserve the redaction goal by never returning the raw segment.
In `@crates/ironclaw_host_runtime/src/turn_scheduler/latency.rs`:
- Around line 60-88: Gate the ScopeFields materialization in operation_ok and
operation_error so it only happens when started_at is present; right now
ScopeFields::from_scope(scope) is evaluated before trace_ok/trace_error can
short-circuit, causing unnecessary cloning on the hot path. Update these helpers
in latency.rs to return early when started_at.is_none() or otherwise defer
constructing ScopeFields until after the macro gate, keeping the trace-only work
out of the scheduler path.
- Around line 74-87: The scheduler trace helper currently hardcodes the error
kind, so panics and normal executor failures are reported the same way. Update
operation_error in turn_scheduler/latency.rs to accept and forward the
caller-provided error_kind instead of always emitting executor_error, or split
out a dedicated panic helper. Then ensure executor_task::result_to_outcome
passes executor_error on the normal failure path and scheduler_executor_panic on
the unwind path so traces preserve the distinct failure modes.
---
Outside diff comments:
In `@crates/ironclaw_host_runtime/src/production.rs`:
- Around line 440-471: Early policy- and trust-rejected exits in
invoke_capability only record the stage-specific trace and skip the top-level
invoke_capability latency trace. Update the early-return branches in
invoke_capability around enforce_runtime_policy and evaluate_invocation_trust to
also emit the overall trace using total_started_at, matching the auth_required
path and preserving a complete latency timeline for fast-fail cases.
In `@crates/ironclaw_reborn_composition/src/runtime/latency.rs`:
- Around line 1-41: The reborn_runtime latency helpers are only tagging
thread_id and run_id, so they lose tenant/agent/project/mission scope needed for
correlation. Update trace_runtime_latency_ok and trace_runtime_latency_error to
accept the full scope data and pass those fields into the
live_latency_trace_ok!/live_latency_trace_error! macros, matching the patterns
used in production.rs, process_executor.rs, and coordinator.rs. Then thread the
already-computed scope from runtime.rs (for example from submit_user_turn via
turn_scope_for) through the call sites so both helpers emit the complete scope.
🪄 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: 71a06843-5db7-48a7-bb91-1e5cf307f25e
📒 Files selected for processing (15)
crates/ironclaw_filesystem/Cargo.tomlcrates/ironclaw_filesystem/src/scoped.rscrates/ironclaw_host_runtime/src/production.rscrates/ironclaw_host_runtime/src/services/process_executor.rscrates/ironclaw_host_runtime/src/turn_scheduler.rscrates/ironclaw_host_runtime/src/turn_scheduler/executor_task.rscrates/ironclaw_host_runtime/src/turn_scheduler/latency.rscrates/ironclaw_observability/src/lib.rscrates/ironclaw_reborn/Cargo.tomlcrates/ironclaw_reborn/src/model_gateway.rscrates/ironclaw_reborn/src/turn_run_executor.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/latency.rscrates/ironclaw_turns/Cargo.tomlcrates/ironclaw_turns/src/coordinator.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_host_runtime/src/turn_scheduler/latency.rs (1)
122-143: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winGate
claim_next_run_result'sOk(Some(...))arm onstarted_attoo.
operation_ok/operation_errorwere just fixed to early-return onstarted_at.is_none()before buildingScopeFields. This function'sOk(Some(claimed))arm still callsScopeFields::from_scope(&claimed.state.scope)unconditionally, reintroducing the same trace-only allocation on the scheduler hot path the PR objective calls out: "avoids trace-only formatting work whenironclaw_latency=traceis disabled." TheOk(None)/Errarms correctly rely on a pre-gatedscope_filter, but theOk(Some)arm sidesteps that gating.🔧 Proposed fix
pub(super) fn claim_next_run_result<E>( scope_filter: Option<&ScopeFields>, started_at: Option<Instant>, claim: &Result<Option<ClaimedTurnRun>, E>, ) { + if started_at.is_none() { + return; + } match claim { Ok(Some(claimed)) => trace_ok(🤖 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 `@crates/ironclaw_host_runtime/src/turn_scheduler/latency.rs` around lines 122 - 143, The Ok(Some(claimed)) branch in claim_next_run_result still builds ScopeFields unconditionally, so add the same started_at.is_none() early return used in operation_ok and operation_error before calling trace_ok. Reuse the existing scope_filter when tracing is disabled, and only construct ScopeFields::from_scope(&claimed.state.scope) when started_at is Some so the scheduler hot path avoids trace-only formatting work.
🤖 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 `@crates/ironclaw_filesystem/src/scoped.rs`:
- Around line 54-61: Add a unit test for scoped_path_class to lock in the
redaction bucket mapping contract. Cover the known buckets returned by
scoped_path_class for workspace, memory, artifacts, and turns, and also verify
an arbitrary identifier-bearing path like /users/alice falls back to other.
Place the test near scoped_path_class so future changes to the path bucketing
logic are caught immediately.
- Around line 54-61: The scoped path bucket logic in scoped_path_class currently
returns bare &'static str literals for a fixed set of categories; replace this
with a typed PathClass enum and have scoped_path_class return that enum instead
of matching on string literals. Add serde support with #[serde(rename_all =
"snake_case")] and update any callers/tests that consume the class value so the
fixed set is represented as an enum consistently.
---
Outside diff comments:
In `@crates/ironclaw_host_runtime/src/turn_scheduler/latency.rs`:
- Around line 122-143: The Ok(Some(claimed)) branch in claim_next_run_result
still builds ScopeFields unconditionally, so add the same started_at.is_none()
early return used in operation_ok and operation_error before calling trace_ok.
Reuse the existing scope_filter when tracing is disabled, and only construct
ScopeFields::from_scope(&claimed.state.scope) when started_at is Some so the
scheduler hot path avoids trace-only formatting work.
🪄 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: bbb3ce03-26b6-4cfc-8b7b-a719c21b3e4b
📒 Files selected for processing (3)
crates/ironclaw_filesystem/src/scoped.rscrates/ironclaw_host_runtime/src/turn_scheduler/executor_task.rscrates/ironclaw_host_runtime/src/turn_scheduler/latency.rs
| fn scoped_path_class(path: &ScopedPath) -> &'static str { | ||
| match path.as_str().split('/').nth(1) { | ||
| Some("workspace") => "workspace", | ||
| Some("memory") => "memory", | ||
| Some("artifacts") => "artifacts", | ||
| Some("turns") => "turns", | ||
| _ => "other", | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Add a unit test locking in the redaction bucket contract.
This function has flip-flopped across three review rounds (raw segment → no-op constant → fixed buckets) on a security-relevant redaction boundary. A small test asserting workspace/memory/artifacts/turns map correctly and an arbitrary/identifier-bearing segment (e.g. /users/alice) maps to other would prevent regressions without needing another review cycle.
🤖 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 `@crates/ironclaw_filesystem/src/scoped.rs` around lines 54 - 61, Add a unit
test for scoped_path_class to lock in the redaction bucket mapping contract.
Cover the known buckets returned by scoped_path_class for workspace, memory,
artifacts, and turns, and also verify an arbitrary identifier-bearing path like
/users/alice falls back to other. Place the test near scoped_path_class so
future changes to the path bucketing logic are caught immediately.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
Consider a typed PathClass enum instead of bare &'static str literals.
Functionally correct now (fixed bucket set, no raw segment leak — matches the prior review's suggested fix). But per coding guidelines, fixed small value sets should be enums rather than string-literal matching.
♻️ Optional typed refactor
-fn scoped_path_class(path: &ScopedPath) -> &'static str {
- match path.as_str().split('/').nth(1) {
- Some("workspace") => "workspace",
- Some("memory") => "memory",
- Some("artifacts") => "artifacts",
- Some("turns") => "turns",
- _ => "other",
- }
+enum PathClass {
+ Workspace,
+ Memory,
+ Artifacts,
+ Turns,
+ Other,
+}
+
+impl PathClass {
+ fn as_str(&self) -> &'static str {
+ match self {
+ Self::Workspace => "workspace",
+ Self::Memory => "memory",
+ Self::Artifacts => "artifacts",
+ Self::Turns => "turns",
+ Self::Other => "other",
+ }
+ }
+}
+
+fn scoped_path_class(path: &ScopedPath) -> PathClass {
+ match path.as_str().split('/').nth(1) {
+ Some("workspace") => PathClass::Workspace,
+ Some("memory") => PathClass::Memory,
+ Some("artifacts") => PathClass::Artifacts,
+ Some("turns") => PathClass::Turns,
+ _ => PathClass::Other,
+ }
}As per coding guidelines, "Fixed small sets of values must be represented as enums with #[serde(rename_all = "snake_case")]... never compare against string literals like status == "in_progress"."
🤖 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 `@crates/ironclaw_filesystem/src/scoped.rs` around lines 54 - 61, The scoped
path bucket logic in scoped_path_class currently returns bare &'static str
literals for a fixed set of categories; replace this with a typed PathClass enum
and have scoped_path_class return that enum instead of matching on string
literals. Add serde support with #[serde(rename_all = "snake_case")] and update
any callers/tests that consume the class value so the fixed set is represented
as an enum consistently.
Source: Coding guidelines
This branch was successfully deployed
1 participant
Summary
Adds live-only latency tracing under the
ironclaw_latencytarget to debug instances where requests stall together and then resolve together.This instruments the Reborn submit path, turn coordinator, scheduler claim/execution, Reborn executor, model gateway, host runtime capability invocation, process execution, and scoped filesystem operations. Filesystem events intentionally emit only a redacted
path_classrather than full virtual paths.The follow-up cleanup centralizes the trace target/elapsed-time helpers in
ironclaw_observability, guards trace-only formatting whenironclaw_latency=traceis disabled, and splits scheduler trace plumbing out ofturn_scheduler.rsso the branch does not push that file past 1k lines.Operator usage
Enable short diagnostic windows with:
See
docs/internal/live-latency-instrumentation.mdfor the interpretation map.Validation
cargo fmtcargo check -p ironclaw_observability -p ironclaw_filesystem -p ironclaw_turns -p ironclaw_host_runtime -p ironclaw_reborn -p ironclaw_reborn_compositioncargo test -p ironclaw_observability -p ironclaw_filesystem -p ironclaw_turns -p ironclaw_host_runtime --libgit diff --check origin/main...HEADNote:
cargo checkstill reports the pre-existingLOCAL_DEV_DB_FILENAMEdead-code warning inironclaw_reborn_composition/src/factory.rs.