From b4093724f225531772b3c4bfe1893ed912e73ff6 Mon Sep 17 00:00:00 2001 From: Hakan Seven Date: Thu, 25 Jun 2026 00:16:08 +0300 Subject: [PATCH] fix(plugin): f64 pick coords + harden out-of-process lifecycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups after merging the out-of-process plugin isolation work: - Convert the out-of-process interactive adapter's `on_point` / `on_entity_pick` to `glam::DVec3`, matching the `CadCommand` trait, which moved to f64 pick coordinates. The branch predated that change and no longer compiled against the trait. - Contain plugin constructor panics in `export_plugin!`: the constructor runs across the C ABI boundary in `ocs_plugin_register`, where an unwinding panic is undefined behavior. Catch it and return null, which the loader already treats as a failed registration. - Reap killed runner processes. The spawn-timeout and disconnect paths called `child.kill()` without `wait()`, leaving a zombie on Unix until the host exited. Route both — and `shutdown()` — through a shared `reap()` that kills and waits on a detached thread. Co-Authored-By: Claude Opus 4.8 --- Cargo.lock | 192 ++++++++++++++++++++++++++- crates/ocs_plugin_api/src/host.rs | 15 ++- crates/ocs_plugin_api/src/process.rs | 23 ++-- src/app/plugin_host.rs | 8 +- 4 files changed, 220 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 612d03509..06fa8a594 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -29,7 +29,6 @@ dependencies = [ "image", "inventory", "js-sys", - "libloading", "lzma-sys", "ocs_plugin_api", "open", @@ -72,7 +71,7 @@ name = "acadrust" version = "0.3.4" source = "git+https://github.com/HakanSeven12/acadrust?branch=main#7b6bbf4a620540e917d6254b1c87076f76909973" dependencies = [ - "ahash", + "ahash 0.8.12", "anyhow", "bitflags 2.11.1", "byteorder", @@ -84,6 +83,7 @@ dependencies = [ "nom 7.1.3", "once_cell", "ryu", + "serde", "thiserror 1.0.69", ] @@ -104,6 +104,17 @@ dependencies = [ "cpufeatures", ] +[[package]] +name = "ahash" +version = "0.7.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "891477e0c6a8957309ee5c45a6368af3ae14bb510732d2684ffa19af310920f9" +dependencies = [ + "getrandom 0.2.17", + "once_cell", + "version_check", +] + [[package]] name = "ahash" version = "0.8.12" @@ -694,6 +705,9 @@ name = "bitflags" version = "2.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c4512299f36f043ab09a583e57bceb5a5aab7a73db1805848e8fef3c9e8c78b3" +dependencies = [ + "serde_core", +] [[package]] name = "bitreader" @@ -713,6 +727,18 @@ dependencies = [ "no_std_io2", ] +[[package]] +name = "bitvec" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ddcec3d12c579d40898fe0a9a358a803c23e9c52ca3c425707f81c9436211837" +dependencies = [ + "funty", + "radium", + "tap", + "wyz", +] + [[package]] name = "block" version = "0.1.6" @@ -799,6 +825,28 @@ version = "3.20.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "72f5acc6cb2ba439de613abc23857ec3d78374d8ed5ac84e9d11336e87da8649" +[[package]] +name = "bytecheck" +version = "0.6.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23cdc57ce23ac53c931e88a43d06d070a6fd142f2617be5855eb75efc9beb1c2" +dependencies = [ + "bytecheck_derive", + "ptr_meta", + "simdutf8", +] + +[[package]] +name = "bytecheck_derive" +version = "0.6.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3db406d29fbcd95542e92559bed4d8ad92636d1ca8b3b72ede10b4bcc010e659" +dependencies = [ + "proc-macro2", + "quote", + "syn 1.0.109", +] + [[package]] name = "bytecount" version = "0.6.9" @@ -1382,6 +1430,12 @@ dependencies = [ "libloading", ] +[[package]] +name = "doctest-file" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c2db04e74f0a9a93103b50e90b96024c9b2bdca8bce6a632ec71b88736d3d359" + [[package]] name = "document-features" version = "0.2.12" @@ -1730,6 +1784,12 @@ version = "0.4.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7ab85b9b05e3978cc9a9cf8fea7f01b494e1a09ed3037e16ba39edc7a29eb61a" +[[package]] +name = "funty" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e6d5a32815ae3f33302d95fdcb2ce17862f8c65363dcfd29360480ba1001fc9c" + [[package]] name = "futf" version = "0.1.5" @@ -2092,6 +2152,15 @@ dependencies = [ "smallvec", ] +[[package]] +name = "hashbrown" +version = "0.12.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8a9ee70c43aaf417c914396645a0fa852624801b24ebb7ae78fe8272889ac888" +dependencies = [ + "ahash 0.7.8", +] + [[package]] name = "hashbrown" version = "0.15.5" @@ -2508,6 +2577,19 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "interprocess" +version = "2.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "069323743400cb7ab06a8fe5c1ed911d36b6919ec531661d034c89083629595b" +dependencies = [ + "doctest-file", + "libc", + "recvmsg", + "widestring", + "windows-sys 0.61.2", +] + [[package]] name = "inventory" version = "0.3.24" @@ -3781,6 +3863,13 @@ name = "ocs_plugin_api" version = "0.1.0" dependencies = [ "acadrust", + "bincode", + "interprocess", + "libloading", + "memmap2", + "rkyv", + "serde", + "thiserror 1.0.69", ] [[package]] @@ -4281,6 +4370,26 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "ptr_meta" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0738ccf7ea06b608c10564b31debd4f5bc5e197fc8bfe088f68ae5ce81e7a4f1" +dependencies = [ + "ptr_meta_derive", +] + +[[package]] +name = "ptr_meta_derive" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "16b845dbfca988fa33db069c0e230574d15a3088f147a87b64c7589eb662c9ac" +dependencies = [ + "proc-macro2", + "quote", + "syn 1.0.109", +] + [[package]] name = "pxfm" version = "0.1.29" @@ -4342,6 +4451,12 @@ version = "6.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f8dcc9c7d52a811697d2151c701e0d08956f92b0e24136cf4cf27b57a6a0d9bf" +[[package]] +name = "radium" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dc33ff2d4973d518d823d61aa239014831e521c75da58e3df4840d3f47749d09" + [[package]] name = "rand" version = "0.7.3" @@ -4576,6 +4691,12 @@ dependencies = [ "font-types 0.11.3", ] +[[package]] +name = "recvmsg" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d3edd4d5d42c92f0a659926464d4cce56b562761267ecf0f469d85b7de384175" + [[package]] name = "redox_syscall" version = "0.4.1" @@ -4632,6 +4753,15 @@ version = "0.8.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "dc897dd8d9e8bd1ed8cdad82b5966c3e0ecae09fb1907d58efaa013543185d0a" +[[package]] +name = "rend" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "71fe3824f5629716b1589be05dacd749f6aa084c87e00e016714a8cdfccc997c" +dependencies = [ + "bytecheck", +] + [[package]] name = "renderdoc-sys" version = "1.1.0" @@ -4705,6 +4835,35 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "rkyv" +version = "0.7.46" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2297bf9c81a3f0dc96bc9521370b88f054168c29826a75e89c55ff196e7ed6a1" +dependencies = [ + "bitvec", + "bytecheck", + "bytes", + "hashbrown 0.12.3", + "ptr_meta", + "rend", + "rkyv_derive", + "seahash", + "tinyvec", + "uuid", +] + +[[package]] +name = "rkyv_derive" +version = "0.7.46" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "84d7b42d4b8d06048d3ac8db0eb31bcb942cbeb709f0b5f2b2ebde398d3038f5" +dependencies = [ + "proc-macro2", + "quote", + "syn 1.0.109", +] + [[package]] name = "robust" version = "1.2.0" @@ -4923,6 +5082,12 @@ dependencies = [ "tiny-skia", ] +[[package]] +name = "seahash" +version = "4.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1c107b6f4780854c8b126e228ea8869f4d7b71260f962fefb57b996b8959ba6b" + [[package]] name = "selectors" version = "0.22.0" @@ -5463,6 +5628,12 @@ dependencies = [ "slotmap", ] +[[package]] +name = "tap" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "55937e1799185b12863d447f42597ed69d9928686b8d88a1df17376a097d8369" + [[package]] name = "tempfile" version = "3.27.0" @@ -6627,6 +6798,12 @@ dependencies = [ "safe_arch", ] +[[package]] +name = "widestring" +version = "1.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "72069c3113ab32ab29e5584db3c6ec55d416895e60715417b5b883a357c3e471" + [[package]] name = "winapi" version = "0.3.9" @@ -6943,7 +7120,7 @@ version = "0.30.13" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a6755fa58a9f8350bd1e472d4c3fcc25f824ec358933bba33306d0b63df5978d" dependencies = [ - "ahash", + "ahash 0.8.12", "android-activity", "atomic-waker", "bitflags 2.11.1", @@ -7111,6 +7288,15 @@ dependencies = [ "wasmparser", ] +[[package]] +name = "wyz" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "05f360fc0b24296329c78fda852a1e9ae82de9cf7b27dae4b7f62f118f77b9ed" +dependencies = [ + "tap", +] + [[package]] name = "x11-dl" version = "2.21.0" diff --git a/crates/ocs_plugin_api/src/host.rs b/crates/ocs_plugin_api/src/host.rs index ac17de820..a048c39ca 100644 --- a/crates/ocs_plugin_api/src/host.rs +++ b/crates/ocs_plugin_api/src/host.rs @@ -97,9 +97,18 @@ macro_rules! export_plugin { #[no_mangle] pub extern "C" fn ocs_plugin_register( ) -> *mut ::std::boxed::Box { - let plugin: ::std::boxed::Box = - ::std::boxed::Box::new($ctor); - ::std::boxed::Box::into_raw(::std::boxed::Box::new(plugin)) + // The constructor runs across a C ABI boundary; a panic unwinding + // past it is undefined behavior. Contain it and report failure as a + // null pointer, which the host loader treats as "registration + // failed" rather than crashing the runner process. + match ::std::panic::catch_unwind(::std::panic::AssertUnwindSafe(|| { + let plugin: ::std::boxed::Box = + ::std::boxed::Box::new($ctor); + ::std::boxed::Box::into_raw(::std::boxed::Box::new(plugin)) + })) { + ::std::result::Result::Ok(ptr) => ptr, + ::std::result::Result::Err(_) => ::std::ptr::null_mut(), + } } }; } diff --git a/crates/ocs_plugin_api/src/process.rs b/crates/ocs_plugin_api/src/process.rs index bae6ef873..32a71a5e6 100644 --- a/crates/ocs_plugin_api/src/process.rs +++ b/crates/ocs_plugin_api/src/process.rs @@ -77,7 +77,7 @@ impl PluginProcess { // Create the listener before spawning so the runner can connect immediately. let listener = ListenerOptions::new().name(socket_name_ref).create_sync()?; - let mut child = Command::new(&runner_path) + let child = Command::new(&runner_path) .arg("--ocs-plugin-runner") .arg(&socket_name) .arg(cdylib_path) @@ -99,11 +99,11 @@ impl PluginProcess { } Ok(Err(e)) => return Err(e.into()), Err(mpsc::RecvTimeoutError::Timeout) => { - let _ = child.kill(); + reap(child); return Err(PluginError::SpawnTimeout(spawn_timeout())); } Err(mpsc::RecvTimeoutError::Disconnected) => { - let _ = child.kill(); + reap(child); return Err(PluginError::RunnerExited); } }; @@ -244,11 +244,8 @@ impl PluginProcess { pub fn shutdown(&self) { let (stream, child) = self.take_resources(); drop(stream); - if let Some(mut child) = child { - let _ = child.kill(); - std::thread::spawn(move || { - let _ = child.wait(); - }); + if let Some(child) = child { + reap(child); } } @@ -287,6 +284,16 @@ impl PluginProcess { } } +/// Kill a child process and reap it without blocking the caller. The blocking +/// `wait()` runs in a detached thread so the host never stalls on a plugin, and +/// the child is reaped rather than left as a zombie on Unix. +fn reap(mut child: Child) { + let _ = child.kill(); + std::thread::spawn(move || { + let _ = child.wait(); + }); +} + fn shutdown_error() -> PluginError { PluginError::Io(std::io::Error::new( std::io::ErrorKind::NotConnected, diff --git a/src/app/plugin_host.rs b/src/app/plugin_host.rs index 098b1e085..db40d3ed8 100644 --- a/src/app/plugin_host.rs +++ b/src/app/plugin_host.rs @@ -333,13 +333,13 @@ impl crate::command::CadCommand for PluginProcessInteractiveAdapter { fn prompt(&self) -> String { self.prompt.clone().unwrap_or_default() } - fn on_point(&mut self, pt: glam::Vec3) -> crate::command::CmdResult { + fn on_point(&mut self, pt: glam::DVec3) -> crate::command::CmdResult { use ocs_plugin_api::ipc::protocol::InteractiveEvent; let result = self .process .interactive_event( self.command_id, - InteractiveEvent::Point([pt.x as f64, pt.y as f64, pt.z as f64]), + InteractiveEvent::Point([pt.x, pt.y, pt.z]), ) .map(plugin_step_to_result) .unwrap_or(crate::command::CmdResult::Cancel); @@ -359,7 +359,7 @@ impl crate::command::CadCommand for PluginProcessInteractiveAdapter { fn needs_entity_pick(&self) -> bool { self.needs_entity_pick.unwrap_or(false) } - fn on_entity_pick(&mut self, handle: Handle, pt: glam::Vec3) -> crate::command::CmdResult { + fn on_entity_pick(&mut self, handle: Handle, pt: glam::DVec3) -> crate::command::CmdResult { use ocs_plugin_api::ipc::protocol::InteractiveEvent; let result = self .process @@ -367,7 +367,7 @@ impl crate::command::CadCommand for PluginProcessInteractiveAdapter { self.command_id, InteractiveEvent::ObjectPick { handle, - pt: [pt.x as f64, pt.y as f64, pt.z as f64], + pt: [pt.x, pt.y, pt.z], }, ) .map(plugin_step_to_result)