feat(vm-runner): out-of-process wasm compilation daemon - #16067
feat(vm-runner): out-of-process wasm compilation daemon#16067jakmeier wants to merge 12 commits into
Conversation
(cherry picked from commit a551a34)
Sandboxed, memory-limited compilation for Wasmtime on Linux. This commit only adds the daemon, actual usage is in the next.
Use the compiler daemon introduced in the previous commit. This is meant to be an optional setup that lives side-by-side with in-process compilation as an experiment for now.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #16067 +/- ##
==========================================
- Coverage 73.45% 73.40% -0.06%
==========================================
Files 861 868 +7
Lines 190459 191204 +745
Branches 190459 191204 +745
==========================================
+ Hits 139910 140353 +443
- Misses 46100 46374 +274
- Partials 4449 4477 +28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Regarding the chosen size limits, I ran experiments on jakmeier/experiments_on_wasmtime_memory_usage against >3000 mainnet contracts. Results show that on average, more than 6 threads doesn't meaningfully reduce compilation time. Virtual memory usage (what is limited by RLIMIT_AS in the PR) does keep increasing with more allocated threads though. Hence I chose 6 threads per worker so we don't have to overallocate too much. With 6 threads, maximum virtual memory usage is 596.46 MB for mainnet contracts. (Crafted contracts we identified as being memory hungry actually stay below that, now that we have limits in place and use Winch.) Compilation time
Peak memory usage (RSS)
Peak virtual memory usage
I also plotted compilation time with different thread numbers grouped by effective WASM size, to ensure the global average and mean don't hide a meaningful improvement on large contracts. Looks like 6 threads hits diminishing returns on all size buckets. 
|
I recall we did not converge on this topic during the meeting. I actually favor the currently implemented option, but that's definitely something which is worth a deeper dive. |
Greptile SummaryAdds an optional out-of-process Wasmtime compilation service.
Confidence Score: 4/5This PR is not safe to merge until daemon failures stop panicking contract execution and the outstanding worker-progress and memory-containment failures are resolved. Production response reads can still retain workers indefinitely, failed RLIMIT_AS setup still permits unrestricted compilation despite the author's reply preferring to defer OOM work, and exhausting daemon retries now reaches an unconditional panic during function execution. Files Needing Attention: runtime/near-vm-runner/src/compiler_daemon/parent.rs, runtime/near-vm-runner/src/compiler_daemon/child.rs, runtime/runtime/src/function_call.rs
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This is necessary to prevent a hanging compilation from blocking workers and eventually neard as a whole.
|
@darioush @ssavenko-near This is now updated, as discussed, to skip chunk endorsement rather than commit I've also added an IPC timeout to avoid potential hangs, as pointed out by the AI review. Everything stays hidden behind a CLI flag. I wouldn't recommend anybody using this until the follow-ups are done and even then I wouldn't rush to enable this by default. @darioush Can you please review this when you can find the time? I would like to merge the basic sandboxing first and address follow-ups mentioned in the PR description with future PRs. |
This is to preserve the current behavior as closely as possible for the first itertation.
Remove superfluous test `background_compilation_does_not_block_critical_pool`. It didn't test anything meaningful. Remove unnecessary 1 * GB.
|
I've changed failure management again according to the latest discussions. Now it crashes the node when compilation doesn't work. Please excuse the git history getting a bit messy. I didn't it clean up and force-pushed this time, since people already started reviewing. The CI failure is unrelated to my changes and fixed in #16153 . |
| Err(VMRunnerError::WasmCompilationUnknownError { debug_message }) => { | ||
| panic!("wasm compilation unknown error: {debug_message}"); | ||
| } |
There was a problem hiding this comment.
When both daemon compilation attempts fail because workers crash, time out, or return IPC errors, compile_in_subprocess returns WasmCompilationUnknownError, which this match arm handles with an unconditional panic. A repeatable worker failure during contract execution can therefore terminate the node instead of aborting or skipping only the affected state transition.
There was a problem hiding this comment.
Nice work :)
High level comments:
- I lean towards 1 binary if possible. This means we don't have to change deploy strategy and there is no possibility of version mismatch between daemon process and neard parent. Also we don't have to worry about finding the binary. One issue here can be jemalloc, which could interfere with memory sandboxing or require some additional settings. Maybe like:
.env("MALLOC_CONF", "narenas:1,background_thread:false,retain:false,dirty_decay_ms:0,muzzy_decay_ms:0") - I think this feature needs a config that can disable it
- For
seccompI suggest leaving any further hardening to a separate PR.
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| metadata.permissions().mode() & 0o111 != 0 |
There was a problem hiding this comment.
maybe we should check something like below?
fn is_usable_compiler_daemon_binary(path: &Path) -> bool {
fs::metadata(path).is_ok_and(|m| m.is_file())
&& accessat(CWD, path, Access::EXEC_OK, AtFlags::EACCESS).is_ok()
}
| /// Number of distinct priority classes; used to size per-class structures. | ||
| // Only read by the compiler-daemon pool, which is gated on `wasmtime_vm`. | ||
| #[cfg_attr(not(feature = "wasmtime_vm"), allow(dead_code))] | ||
| pub(crate) const COUNT: usize = 3; |
There was a problem hiding this comment.
nit: maybe a UT can tie this to the enum
| /// | ||
| /// Errors from either compile are dropped. | ||
| /// | ||
| /// Uses `CompilePriority::Critical` if the out-of-process compiler is enabled. |
There was a problem hiding this comment.
nit: Maybe worth clarifying that the next_config compilation uses the background priority
| // reach these arms. | ||
| // | ||
| // TODO: Compilation must become asynchronous before this can | ||
| // work with SPICE, where validators endorse before execution. |
There was a problem hiding this comment.
| // work with SPICE, where validators endorse before execution. | |
| // work with SPICE, where chunks are determined before execution. |
| //! [`near_vm_runner::compiler_daemon::daemon_main`], making it easy for neard | ||
| //! to include a command that directly calls it without the need for a second | ||
| //! binary. |
There was a problem hiding this comment.
| //! [`near_vm_runner::compiler_daemon::daemon_main`], making it easy for neard | |
| //! to include a command that directly calls it without the need for a second | |
| //! binary. | |
| //! [`near_vm_runner::compiler_daemon::daemon_main`], which is public so neard | |
| //! can call it without the need for an additional binary. |
is this what is intended? a bit confused because seems there is a second binary.
| ) -> Option<PathBuf> { | ||
| if let Some(configured_path) = configured_path { | ||
| let configured_path = if configured_path.is_relative() { | ||
| home_dir.join(configured_path) |
There was a problem hiding this comment.
This join can produce a relative path, since --home is taken verbatim with no canonicalize (neard/src/cli.rs:203-205). Two ways that ends badly, both in the same place.
The checked file is not the executed file. is_usable_compiler_daemon_binary resolves a relative path against the process cwd through fs::metadata, but the spawn sets .current_dir("/") (parent.rs:80), and on Unix the chdir happens before the exec. std calls out exactly this case as platform specific and unstable, and recommends canonicalizing instead (library/std/src/process.rs:936-940).
So neard --home ./localnet/node0 with compiler_daemon_binary_path: "bin/daemon" passes the usability check on ./localnet/node0/bin/daemon and then execs /localnet/node0/bin/daemon. Since the check passed, set_daemon_binary has already run, so is_daemon_configured() is true and every compile routes to a daemon that can never start: two failed spawn attempts, WasmCompilationUnknownError, and a panic under panic = 'abort'. A relative --home would kill the node on the first contract call.
An empty home turns the path into a $PATH lookup. get_default_home() returns an empty path when both NEAR_HOME and HOME are unset (lib.rs:83-94), which happens in containers and bare systemd units. Path::new("").join("near-vm-compiler-daemon") is a bare name with no separator, so exec resolves it through PATH rather than from the file that was checked. env_clear() makes that resolution less predictable, not more.
Both go away by canonicalizing here and refusing a result that is not absolute.
🤖 -- Claude on behalf of Darioush
| while let Some(worker) = inner.idle.pop() { | ||
| if worker.is_alive() { | ||
| inner.waiters[idx] -= 1; | ||
| if !inner.idle.is_empty() { | ||
| self.wake_one(&inner); | ||
| } | ||
| return Ok(worker); | ||
| } | ||
| // Dead idle worker: release its permit and let it drop (reaped). | ||
| inner.live -= 1; | ||
| } |
There was a problem hiding this comment.
This drain loop frees a permit per dead worker (inner.live -= 1) but never calls wake_one. The caller then consumes exactly one of those permits at step 2. So if N dead workers are drained, N-1 permits become available with no waiter notified.
This looks reachable by design rather than by accident: raise_oom_score_adj in child.rs:203-209 marks workers as the kernel's preferred OOM victims, so several idle workers dying together is the expected behavior under memory pressure. That is exactly when the freed permits matter most.
It is not a permanent stall since the next check_in/discard calls wake_one again. But production requests have no deadline (compilation_request_timeout returns None, parent.rs:185-190), so the waiters stay blocked for as long as the draining caller's compile takes.
Would something like this work?
let mut freed_permits = false;
while let Some(worker) = inner.idle.pop() {
if worker.is_alive() {
inner.waiters[idx] -= 1;
if !inner.idle.is_empty() {
self.wake_one(&inner);
}
return Ok(worker);
}
inner.live -= 1;
freed_permits = true;
}
if freed_permits {
self.wake_one(&inner);
}Separately, the dead worker drops at the end of each iteration while inner is still locked. DaemonProcess::drop joins the watchdog thread, kills and wait()s the child, and joins the stderr thread, all under the pool mutex. Moving the dead workers into a local Vec and dropping them after the lock is released would match what Lease::discard already does.
🤖 -- Claude on behalf of Darioush
| /// Per-request retry budget on IPC failure (for example, a worker crash). | ||
| const MAX_SPAWN_ATTEMPTS: u32 = 2; |
There was a problem hiding this comment.
nit: the loop at parent.rs:452 counts request attempts, not spawns. An IPC error on a live worker consumes the budget too. MAX_REQUEST_ATTEMPTS?
🤖 -- Claude on behalf of Darioush
| //! A pool of worker subprocesses serves compilations in parallel, so | ||
| //! independent compilations shards run concurrently with independent memory | ||
| //! limits . The pool spawns workers lazily up to a configured maximum and |
There was a problem hiding this comment.
nit: "compilations shards" looks like a word is missing, and there is a stray space before the period on limits .
🤖 -- Claude on behalf of Darioush
| Err(VMRunnerError::WasmCompilationUnknownError { debug_message }) => { | ||
| panic!("wasm compilation unknown error: {debug_message}"); | ||
| } |
There was a problem hiding this comment.
The panic reads right for chunk application: stopping beats committing a state transition that other nodes would compute differently.
The concern is the other caller. state_viewer/mod.rs:511 runs this same function for RPC view calls with view_config: Some(..). There is no state transition to protect there and no endorsement to withhold, just a query answer. So an unauthenticated call_function against a contract that is not in the compiled cache reaches this arm, and panic = 'abort' in [profile.release] means the process dies rather than unwinds. A kernel without Landlock gets here on every compile too, since sandbox::apply fails and the child exits (child.rs:31).
Could we keep the panic for apply only?
Err(VMRunnerError::WasmCompilationUnknownError { debug_message })
if context.view_config.is_none() =>
{
panic!("wasm compilation unknown error: {debug_message}");
}
Err(VMRunnerError::WasmCompilationUnknownError { debug_message }) => {
return Ok(VMOutcome::nop_outcome(FunctionCallError::WasmUnknownError {
msg: debug_message,
}));
}Two details if you take this. view_config is moved into context at line 279, so the guard needs context.view_config, the way line 353 does it. And the guard makes the match non-exhaustive, hence the second arm. The view caller already turns outcome.aborted into CallFunctionError::VMError (state_viewer/mod.rs:530-537), so nothing new is needed on that side. The error variant is your call, WasmUnknownError is just the closest one that exists today.
🤖 -- Claude on behalf of Darioush
3 participants
Sandboxed, memory-limited compilation for Wasmtime on Linux.
This is meant to be an optional setup that lives side-by-side with in-process compilation as an experiment for now.
The implementation is not complete, to keep the initial PR reasonably sized.
(Hint: Review it commit-by-commit.)
Follow-ups are planned for:
Questions that need to be resolved and might also need follow-ups:
WasmCompilationUnknownError? For now, it results in skipping chunk endorsement.