From b1f4047f9c31979ac9c9a2b0a267fd6c7e2728cb Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Wed, 30 Sep 2026 07:41:39 +0100 Subject: [PATCH] fix(plugins): degrade ensure_plugin_state instead of panicking Bug: The typed ensure_plugin_state helper and the object-safe trait method it wraps turned two recoverable conditions into process deaths. On the in-process host, the helper downcast the stored state with .expect("plugin state type mismatch for plugin_id"), so registering state under one plugin_id as one type and later asking for another (e.g. a reloaded plugin version changes the type while the per-tab state map survives) panicked the host. On out-of-process hosts (ipc/client.rs, ipc/v4/client.rs) and on NullHost, the trait method itself was implemented as a bare panic!("ensure_plugin_state is not supported for out-of-process plugins..."), so any out-of-process plugin calling the documented helper crashed its own runner process - even though the sibling plugin_state_any and plugin_state_any_mut methods on those same implementations already degrade to None gracefully. The helper also used .expect("init called once") on the initialiser closure. Reproduce: 1. Out-of-process: a plugin whose runner calls ensure_plugin_state (&mut host, "my.id", ...). 2. Type mismatch: in an in-process plugin, ensure state under one id as u32, then ensure a different type under the same id. 3. Before: step 1 kills the plugin runner at the trait panic; step 2 kills the host at the downcast expect. After: both get None, keep running, and existing state is untouched. Fix: Give the trait method a failure channel: ensure_plugin_state_any now returns Option<&mut (dyn Any + Send + Sync)>, mirroring the sibling plugin_state_any_mut on the same implementations, and the typed helper returns Option<&mut T> (any?.downcast_mut). The panicking implementations (ipc v3, ipc v4, NullHost, and the three never-called test dummies) return None; the in-process app host wraps its real storage in Some with unchanged insert-or-return semantics. The init closure is now guarded by take() with a unit-Any fallback, so even an implementation invoking it twice fails the typed downcast instead of panicking. Object safety, argument types and vtable slot are unchanged; external plugin sources get a compile error at the helper and adopt Option handling on rebuild. Regression tests use catch_unwind so they compile against both signatures: one per panic site (out-of- process, NullHost, type mismatch), each additionally asserting the None result and that pre-existing state survived. --- crates/ocs_plugin_api/src/host.rs | 16 +++++---- crates/ocs_plugin_api/src/ipc/client.rs | 23 ++++++++++-- crates/ocs_plugin_api/src/ipc/v4/client.rs | 6 ++-- crates/ocs_plugin_api/src/process.rs | 24 ++++++++++--- crates/ocs_plugin_api/src/process/manager.rs | 4 +-- crates/ocs_plugin_api/src/process/v4.rs | 4 +-- src/app/plugin_host.rs | 38 ++++++++++++++++---- 7 files changed, 89 insertions(+), 26 deletions(-) diff --git a/crates/ocs_plugin_api/src/host.rs b/crates/ocs_plugin_api/src/host.rs index 4714bb113..5880a0335 100644 --- a/crates/ocs_plugin_api/src/host.rs +++ b/crates/ocs_plugin_api/src/host.rs @@ -833,11 +833,12 @@ pub trait HostApi { fn plugin_state_any(&self, plugin_id: &str) -> Option<&(dyn Any + Send + Sync)>; fn plugin_state_any_mut(&mut self, plugin_id: &str) -> Option<&mut (dyn Any + Send + Sync)>; /// Get the state for `plugin_id`, inserting `init()`'s result if absent. + /// Returns `None` when this host cannot store state (e.g. out-of-process). fn ensure_plugin_state_any( &mut self, plugin_id: &'static str, init: &mut dyn FnMut() -> Box, - ) -> &mut (dyn Any + Send + Sync); + ) -> Option<&mut (dyn Any + Send + Sync)>; // ── DocumentReader (added in API v3; appended at the end to keep vtable // indices stable for API v2 plugins) ───────────────────────────────────── @@ -1159,17 +1160,20 @@ pub fn plugin_state_mut<'a, T: Any + Send + Sync>( } /// Typed get-or-insert of per-tab plugin state stored under `plugin_id`. +/// +/// Returns `None` when this host cannot hold state (out-of-process plugins) +/// or when existing state under `plugin_id` has a different type. pub fn ensure_plugin_state<'a, T: Any + Send + Sync>( host: &'a mut dyn HostApi, plugin_id: &'static str, init: impl FnOnce() -> T, -) -> &'a mut T { +) -> Option<&'a mut T> { let mut init = Some(init); - let any = host.ensure_plugin_state_any(plugin_id, &mut || { - Box::new((init.take().expect("init called once"))()) + let any = host.ensure_plugin_state_any(plugin_id, &mut || match init.take() { + Some(make) => Box::new(make()), + None => Box::new(()), }); - any.downcast_mut::() - .expect("plugin state type mismatch for plugin_id") + any?.downcast_mut::() } #[cfg(feature = "host")] diff --git a/crates/ocs_plugin_api/src/ipc/client.rs b/crates/ocs_plugin_api/src/ipc/client.rs index daf58878d..1d10b1130 100644 --- a/crates/ocs_plugin_api/src/ipc/client.rs +++ b/crates/ocs_plugin_api/src/ipc/client.rs @@ -364,10 +364,11 @@ impl HostApi for PluginHostApi { &mut self, _plugin_id: &'static str, _init: &mut dyn FnMut() -> Box, - ) -> &mut (dyn Any + Send + Sync) { + ) -> Option<&mut (dyn Any + Send + Sync)> { // Same limitation as `plugin_state_any`. This would need a serializable - // state contract to work across processes. - panic!("ensure_plugin_state is not supported for out-of-process plugins; keep state in the plugin crate") + // state contract to work across processes. Degrade to `None` so plugins + // keep running with state unavailable instead of crashing the runner. + None } fn document_reader(&self) -> Box { @@ -633,6 +634,22 @@ mod tests { (api, client_stream) } + #[test] + fn ensure_plugin_state_does_not_panic_for_out_of_process_hosts() { + let (mut api, _peer) = make_client(); + let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let _ = api.ensure_plugin_state_any("opencad.demo", &mut || Box::new(7u32)); + })); + assert!( + outcome.is_ok(), + "out-of-process ensure_plugin_state must degrade, not panic: {:?}", + outcome.err() + ); + assert!(api + .ensure_plugin_state_any("opencad.demo", &mut || Box::new(7u32)) + .is_none()); + } + #[test] fn push_info_emits_request() { let (mut api, mut peer) = make_client(); diff --git a/crates/ocs_plugin_api/src/ipc/v4/client.rs b/crates/ocs_plugin_api/src/ipc/v4/client.rs index 4eeeec8e2..41834fc05 100644 --- a/crates/ocs_plugin_api/src/ipc/v4/client.rs +++ b/crates/ocs_plugin_api/src/ipc/v4/client.rs @@ -607,8 +607,10 @@ impl HostApi for V4PluginHostApi { &mut self, _plugin_id: &'static str, _init: &mut dyn FnMut() -> Box, - ) -> &mut (dyn Any + Send + Sync) { - panic!("ensure_plugin_state is not supported for out-of-process plugins; keep state in the plugin crate") + ) -> Option<&mut (dyn Any + Send + Sync)> { + // Same limitation as `plugin_state_any`; degrade to `None` so the + // plugin keeps running instead of dying on its own helper call. + None } fn document_reader(&self) -> Box { diff --git a/crates/ocs_plugin_api/src/process.rs b/crates/ocs_plugin_api/src/process.rs index 352ba0ccd..760f1353c 100644 --- a/crates/ocs_plugin_api/src/process.rs +++ b/crates/ocs_plugin_api/src/process.rs @@ -128,8 +128,8 @@ impl HostApi for NullHost { &mut self, _plugin_id: &'static str, _init: &mut dyn FnMut() -> Box, - ) -> &mut (dyn std::any::Any + Send + Sync) { - panic!("NullHost: ensure_plugin_state_any not available") + ) -> Option<&mut (dyn std::any::Any + Send + Sync)> { + None } } @@ -1158,6 +1158,22 @@ mod tests { let runner = distinct_runner_path(&host); assert_eq!(runner, PathBuf::from("/app/OpenCADStudio-plugin-runner")); } + + #[test] + fn null_host_ensure_plugin_state_does_not_panic() { + let mut null = NullHost; + let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let _ = null.ensure_plugin_state_any("opencad.demo", &mut || Box::new(7u32)); + })); + assert!( + outcome.is_ok(), + "NullHost ensure_plugin_state must degrade, not panic: {:?}", + outcome.err() + ); + assert!(null + .ensure_plugin_state_any("opencad.demo", &mut || Box::new(7u32)) + .is_none()); + } } #[cfg(all(test, feature = "host"))] @@ -1261,8 +1277,8 @@ mod timeout_tests { &mut self, _plugin_id: &'static str, _init: &mut dyn FnMut() -> Box, - ) -> &mut (dyn std::any::Any + Send + Sync) { - panic!("not used") + ) -> Option<&mut (dyn std::any::Any + Send + Sync)> { + None } } diff --git a/crates/ocs_plugin_api/src/process/manager.rs b/crates/ocs_plugin_api/src/process/manager.rs index 82d4d3e18..90ac264d7 100644 --- a/crates/ocs_plugin_api/src/process/manager.rs +++ b/crates/ocs_plugin_api/src/process/manager.rs @@ -351,8 +351,8 @@ mod tests { &mut self, _plugin_id: &'static str, _init: &mut dyn FnMut() -> Box, - ) -> &mut (dyn std::any::Any + Send + Sync) { - panic!("not used") + ) -> Option<&mut (dyn std::any::Any + Send + Sync)> { + None } } diff --git a/crates/ocs_plugin_api/src/process/v4.rs b/crates/ocs_plugin_api/src/process/v4.rs index 25b1b6d2e..f7e46d02e 100644 --- a/crates/ocs_plugin_api/src/process/v4.rs +++ b/crates/ocs_plugin_api/src/process/v4.rs @@ -468,8 +468,8 @@ mod tests { &mut self, _plugin_id: &'static str, _init: &mut dyn FnMut() -> Box, - ) -> &mut (dyn std::any::Any + Send + Sync) { - panic!("not used") + ) -> Option<&mut (dyn std::any::Any + Send + Sync)> { + None } } diff --git a/src/app/plugin_host.rs b/src/app/plugin_host.rs index d4dfd4481..b8d7acce9 100644 --- a/src/app/plugin_host.rs +++ b/src/app/plugin_host.rs @@ -2612,12 +2612,14 @@ impl HostApi for HostSession<'_> { &mut self, plugin_id: &'static str, init: &mut dyn FnMut() -> Box, - ) -> &mut (dyn Any + Send + Sync) { - self.app.tabs[self.tab] - .plugin_state - .entry(plugin_id) - .or_insert_with(|| init()) - .as_mut() + ) -> Option<&mut (dyn Any + Send + Sync)> { + Some( + self.app.tabs[self.tab] + .plugin_state + .entry(plugin_id) + .or_insert_with(|| init()) + .as_mut(), + ) } fn document_reader(&self) -> Box { Box::new(ocs_plugin_api::host::CadDocumentReader(self.document())) @@ -9157,7 +9159,7 @@ step('undo_move', lambda: M.move([u], (0, 0, 0), (7, 0, 0))) // Absent before first use. assert!(host::plugin_state::(&*host, "opencad.demo").is_none()); // Insert via ensure, then mutate. - *host::ensure_plugin_state(host, "opencad.demo", || 7u32) += 1; + *host::ensure_plugin_state(host, "opencad.demo", || 7u32).unwrap() += 1; assert_eq!( *host::plugin_state::(&*host, "opencad.demo").unwrap(), 8 @@ -9169,6 +9171,28 @@ step('undo_move', lambda: M.move([u], (0, 0, 0), (7, 0, 0))) ); } + #[test] + fn ensure_plugin_state_type_mismatch_does_not_panic() { + use ocs_plugin_api::host::{self, HostApi}; + let mut app = OpenCADStudio::new_for_test(); + let mut session = HostSession::new(&mut app, 0); + let host: &mut dyn HostApi = &mut session; + let _ = host::ensure_plugin_state(host, "opencad.demo", || 7u32); + let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let _ = + host::ensure_plugin_state::(&mut *host, "opencad.demo", || "x".to_string()); + })); + assert!( + outcome.is_ok(), + "type mismatch in ensure_plugin_state must degrade, not panic: {:?}", + outcome.err() + ); + assert!( + host::ensure_plugin_state::(host, "opencad.demo", || "x".to_string()).is_none() + ); + assert_eq!(*host::ensure_plugin_state::(host, "opencad.demo", || 0).unwrap(), 7); + } + #[test] fn add_entities_batch_assigns_handles_and_publishes_once() { let mut app = OpenCADStudio::new_for_test();