fix(lifecycle): settle the state machine when a callback panics

Moving the transition callback out of the state-machine lock left a
worse failure mode than the deadlock it removed. `begin` enters the
intermediate state and drops its guard; if the callback then panics, the
unwind escapes `trigger_transition` before `finish` runs, so `current`
stays at `Configuring`/`Activating`/... forever. Because the guard was
already gone, the mutex is not poisoned either, so nothing reports it:
every later `begin` returns `None` and `~/get_state` answers
`configuring` for the life of the process.

While the callback ran under the guard, the same panic unwound through a
live `MutexGuard` and poisoned the mutex, so the next access failed
loudly. Fail-fast had become a silent permanent wedge.

Catch the unwind, settle on `Failure` — revert to the start state, the
same "nothing changed" outcome as the invalid-transition arm — and
re-raise the payload so the panic is still as loud as before. `on_error`
is deliberately not run on that path: we are already unwinding, and
running more user code on the way out risks a second panic. The
`on_error` invocation gets the same guard, since a panic there would
otherwise strand the node in `ErrorProcessing`.

Author: YuanYuYuan

crates/hiroz-tests/tests/reentrant_lifecycle.rs

@@ -215,3 +215,75 @@ fn failing_transition_callback_still_observes_intermediate_state() {
         assert_eq!(lc_node.get_current_state(), LifecycleState::Inactive);
     });
 }
+
+/// A transition callback that panics must not leave the node wedged.
+///
+/// This is the hazard the `begin`/`finish` split introduced. `begin` moves the
+/// state machine into the intermediate ("busy") state and releases the guard;
+/// the callback then runs unlocked. If it panics, the unwind used to escape
+/// `trigger_transition` before `finish` ran — so `current` stayed at
+/// `Configuring` forever. Worse, because the guard had already been dropped,
+/// the mutex was *not* poisoned, so nothing ever reported it: every later
+/// `begin` returned `None` and `~/get_state` answered `configuring` for the
+/// life of the process.
+///
+/// That is strictly worse than the behaviour it replaced. While the callback
+/// ran under the guard, a panic unwound through a live `MutexGuard` and
+/// poisoned the mutex, so the very next `lock().unwrap()` panicked loudly.
+/// Fail-fast became a silent permanent wedge.
+///
+/// The fix catches the unwind, settles the state machine on `Failure` (revert
+/// to the start state — "the transition did not happen"), and re-raises the
+/// payload, so the panic is still as loud as before while the node stays
+/// usable.
+///
+/// Reverting the `catch_unwind` in `ZLifecycleNode::trigger_transition` makes
+/// both assertions below fail: the state reads `Configuring`, and the recovery
+/// transition returns `Configuring` because `begin` refuses to start.
+#[test]
+#[serial]
+fn panicking_transition_callback_does_not_wedge_the_node() {
+    with_deadline("lifecycle_transition_panic", || {
+        const NODE_NAME: &str = "lc_panicking_callback";
+
+        let router = TestRouter::new();
+        let ctx_node = create_hiroz_context_with_endpoint(router.endpoint()).expect("node ctx");
+        let mut lc_node = ctx_node
+            .create_lifecycle_node(NODE_NAME)
+            .build()
+            .expect("lifecycle node");
+
+        lc_node.on_configure = Box::new(|_prev| panic!("deliberate panic from on_configure"));
+
+        // The panic must still propagate — silently swallowing it would be its
+        // own defect. Keep the default hook quiet for this one call so the
+        // expected backtrace does not look like a test failure.
+        let prev_hook = std::panic::take_hook();
+        std::panic::set_hook(Box::new(|_| {}));
+        let outcome =
+            std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| lc_node.configure()));
+        std::panic::set_hook(prev_hook);
+
+        assert!(
+            outcome.is_err(),
+            "the panic was swallowed; it must still reach the caller"
+        );
+
+        // The node must be back at its pre-transition primary state, not
+        // stranded in the intermediate one.
+        assert_eq!(
+            lc_node.get_current_state(),
+            LifecycleState::Unconfigured,
+            "node wedged in an intermediate state after a panicking callback"
+        );
+
+        // And it must still be usable: a subsequent transition has to work.
+        lc_node.on_configure = Box::new(|_prev| CallbackReturn::Success);
+        let recovered = lc_node.configure().expect("configure after panic");
+        assert_eq!(
+            recovered,
+            LifecycleState::Inactive,
+            "node could not transition after a panicking callback"
+        );
+    });
+}

crates/hiroz/src/lifecycle/node.rs

@@ -1,4 +1,7 @@
-use std::sync::{Arc, Mutex};
+use std::{
+    panic::{AssertUnwindSafe, catch_unwind, resume_unwind},
+    sync::{Arc, Mutex},
+};
 
 use tracing::{debug, info, warn};
 use zenoh::{Result, Wait, query::Query};
@@ -153,7 +156,38 @@ impl ZLifecycleNode {
         let cb_result = match begun {
             Some(start_state) => {
                 // Lock released; observers see the intermediate ("busy") state.
-                let ret = callback(start_state);
+                //
+                // A panic here must not escape without settling the state
+                // machine. `begin` has already moved `current` to the
+                // intermediate state, and the guard it took is gone — so an
+                // unwind straight out of this function would leave `current`
+                // at `Configuring`/`Activating`/... permanently, *and* leave
+                // the mutex unpoisoned, so nothing would ever report it. Every
+                // later `begin` returns `None` and `~/get_state` answers
+                // `configuring` for the life of the process: a silent wedge.
+                // Before the callback was moved out of the lock, the unwind
+                // passed through a live guard and poisoned the mutex, so the
+                // next access failed loudly.
+                //
+                // Settle on `Failure`, which reverts to `start_state` — the
+                // same "nothing changed" outcome as the invalid-transition arm
+                // below, and the only result that is both well-defined and
+                // leaves the node usable. `on_error` is deliberately not run:
+                // we are already unwinding, and running more user code on the
+                // way out would risk a second panic. The payload is then
+                // re-raised, so a panicking callback still fails exactly as
+                // loudly as it did before the split.
+                let ret = match catch_unwind(AssertUnwindSafe(|| callback(start_state))) {
+                    Ok(ret) => ret,
+                    Err(payload) => {
+                        self.state_machine.lock().unwrap().finish(
+                            transition,
+                            start_state,
+                            CallbackReturn::Failure,
+                        );
+                        resume_unwind(payload);
+                    }
+                };
                 self.state_machine
                     .lock()
                     .unwrap()
@@ -164,8 +198,20 @@ impl ZLifecycleNode {
         };
 
         let final_state = if cb_result == State::ErrorProcessing {
-            // Same split for the error path.
-            let ret = (self.on_error)(State::ErrorProcessing);
+            // Same split, and the same unwind guard, for the error path: a
+            // panic in `on_error` would otherwise strand the node in
+            // `ErrorProcessing` with no way out.
+            let cb = AssertUnwindSafe(|| (self.on_error)(State::ErrorProcessing));
+            let ret = match catch_unwind(cb) {
+                Ok(ret) => ret,
+                Err(payload) => {
+                    self.state_machine
+                        .lock()
+                        .unwrap()
+                        .finish_error_processing(CallbackReturn::Error);
+                    resume_unwind(payload);
+                }
+            };
             self.state_machine
                 .lock()
                 .unwrap()