Architectural improvements: localize Script and MCP runtime adapters - #3543
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the system by moving the McpRuntimeAdapter and ScriptRuntimeAdapter implementations, along with their associated error mapping logic, from ironclaw_host_runtime into the ironclaw_mcp and ironclaw_scripts crates. This change centralizes the adapter logic within the specific runtime crates and updates dependencies accordingly. A high-severity issue was identified in the ScriptRuntimeAdapter where a synchronous, blocking call to execute_extension_json is made within an asynchronous function. This can block the async runtime's worker threads, so it is recommended to wrap this operation in tokio::task::spawn_blocking to ensure proper performance and avoid potential deadlocks.
| let execution = self | ||
| .executor | ||
| .execute_extension_json( | ||
| request.governor, | ||
| ScriptExecutionRequest { | ||
| package: request.package, | ||
| capability_id: request.capability_id, | ||
| scope: request.scope, | ||
| estimate: request.estimate, | ||
| mounts: request.mounts, | ||
| resource_reservation: request.resource_reservation, | ||
| invocation: ScriptInvocation { | ||
| input: request.input, | ||
| }, | ||
| }, | ||
| ) | ||
| .map_err(|error| DispatchError::Script { | ||
| kind: script_error_kind(&error), | ||
| })?; |
There was a problem hiding this comment.
The executor.execute_extension_json method is synchronous and performs blocking operations (like waiting for a Docker container). Calling it directly within an async function will block the worker thread of the async runtime. This can lead to performance degradation and potential deadlocks under load.
To fix this, the blocking call should be moved to a dedicated thread pool using tokio::task::spawn_blocking. When implementing this, ensure that the JoinError is logged to capture debugging information, specifically distinguishing between panics and cancellations in the error message.
This will likely require some refactoring to handle the lifetimes of the arguments passed to execute_extension_json, as spawn_blocking requires a 'static closure. A potential approach could involve:
- Changing
ExtensionRegistry::get_extensionto return anArc<ExtensionPackage>. - Creating an owned version of
ScriptExecutionRequestthat can be moved into thespawn_blockingclosure. - Adjusting how the
governoris passed down so an owned handle (like anArc) is available to be moved.
References
- In async functions, use asynchronous I/O operations or spawn_blocking for synchronous operations to avoid blocking the async runtime executor.
- When handling errors from tokio::task::spawn_blocking, log the JoinError and distinguish between panics and cancellations.
zmanian
left a comment
There was a problem hiding this comment.
Review
Summary: Pure code-motion — ScriptRuntimeAdapter + McpRuntimeAdapter + script_error_kind/mcp_error_kind move from ironclaw_host_runtime::services into their owning lane crates (ironclaw_scripts, ironclaw_mcp). Dedupes the test-only adapter copies in integration tests.
Verified
- Trust boundary preserved — host-runtime composition seam at
crates/ironclaw_host_runtime/src/services.rs:37,61just imports from the lane crates; signatures identical;RuntimeAdapter::dispatch_json→ResourceGovernor→RuntimeAdapterResultunchanged. Sandboxed (WASM) vs native (script/MCP) separation intact. - MCP credential handling unchanged — no touchpoints in
RuntimeCredentialInjection,RuntimeHttpEgress, orrequires_host_http_egress(ironclaw_mcp/src/lib.rs:993). Credential injection still flows through the executor; adapter is a thin shim.
Findings (non-blocking)
-
Adapter constructors now
pubacross crates. Both adapters are nowpub structwithpub fn from_executor(crates/ironclaw_mcp/src/lib.rs:946,951;crates/ironclaw_scripts/src/lib.rs:469,474). In the old location they were private toservices.rs. Per the #3460 witness pattern, host-runtime is the only legitimate composer;pub(crate)won't work across crates, but aHostAdapter-witness wrapper or apubconstructor sealed by a private witness type would tighten this. Same formcp_error_kind/script_error_kind— fine as observability helpers but they leak theRuntimeDispatchErrorKindmapping. -
Script test path change worth confirming intentional. Old in-test
ScriptRuntimeAdapterpassedmounts: None. The new shared adapter forwardsrequest.mountsfromRuntimeAdapterRequest. Production-correct (mounts must flow), but the integration test now exercises a different code path than before. Worth confirming intentional. -
#3492 naming criterion not addressed here. Lane crates aren't renamed to
*_native_*/*_host_*. Consistent with the "separate architectural improvements PR" framing — file as follow-up. -
Gemini's blocking-in-async flag on
execute_extension_jsoninScriptRuntimeAdapteris pre-existing (function was already sync, invoked from an async adapter inservices.rs) and out of scope for a code-motion PR. Worth a separate tracked issue.
LGTM.
|
Addressed the non-blocking review follow-ups in
Verification run locally:
CI is passing and the PR remains approved. |
…earai#3543) * refactor(reborn): localize script and mcp runtime adapters * fix(reborn): address zmanian review — seal runtime adapters (nearai#3543)
Summary
ScriptRuntimeAdapter/McpRuntimeAdapterconstructors and public error-kind mapping helpersArchitectural improvements
This remains an architectural cleanup PR for Reborn runtime adapter locality, but now tightens the host-runtime composition boundary per review feedback: lane crates own execution request/client/backend behavior, while
HostRuntimeServicesis the only place that adapts those executors into the neutral dispatcher port.Follow-ups from the review that remain intentionally separate: #3492 lane crate naming criteria and the pre-existing blocking-in-async concern for script execution.
Verification
cargo test -p ironclaw_scripts --lockedcargo test -p ironclaw_mcp --lockedcargo test -p ironclaw_host_runtime --no-default-features --locked --quietcargo test -p ironclaw_architecture --locked --quietcargo clippy -p ironclaw_scripts -p ironclaw_mcp -p ironclaw_host_runtime --no-default-features --tests --locked --quiet -- -D warningscargo clippy -p ironclaw_architecture --tests --locked --quiet -- -D warningscargo fmt --checkgit diff --checkbash scripts/pre-commit-safety.sh(staged diff)