fix(lifecycle): run transition callbacks outside the state-machine lock

YuanYuYuan · YuanYuYuan · commit de0039ecabf0 · 2026-07-28T13:40:05.000+08:00
`ZLifecycleNode::trigger_transition` invoked the user's transition callback
from inside `state_machine.lock().unwrap().trigger(..)`. That mutex is shared
with the node's own `~/get_state`, `~/change_state` and
`~/get_available_transitions` handlers and with `get_current_state()`, so a
node could not answer any question about its own state while a transition
callback ran — and a callback that asks waits forever.

Split `StateMachine::trigger` into `begin` (validate, enter the intermediate
"busy" state) and `finish` (apply the callback's verdict), with `trigger` kept
as a wrapper for callers that own the machine outright. The error path gets the
same `finish_error_processing` split. The callback now runs between the two
locked steps, with no guard held, and observers see the genuine intermediate
state while it runs.

The `begin` call is bound to its own `let` on purpose: writing it as a `match`
scrutinee keeps the temporary guard alive for the whole `match` body and
silently reinstates the deadlock. That mistake was made and caught by these
tests during development.

Also fixes `state_from_lc`, which mapped every transition state onto
`Unconfigured`. A node genuinely reports `configuring` / `activating` while a
callback runs; telling a lifecycle manager it had reset itself instead is wrong
and was indistinguishable from the real thing. Without this the intermediate
state is unobservable over the wire, so the deadlock test could not check it.

Managed entities get the same collect-then-invoke treatment:
`trigger_transition` called `e.on_activate()` / `e.on_deactivate()` while
iterating under `managed_entities.lock()`, which `create_publisher` also takes.
This is not reachable today — the only way to register an entity is
`create_publisher`, so every element is a `ZLifecyclePublisher` whose
activate/deactivate only flip an atomic — and it ships without a test on
purpose, because a test for it would have to fabricate a registration path that
does not exist, and a detector that can only fail against invented code proves
nothing. The comment says exactly that so the next reader does not re-derive it.

Detector evidence, both directions. Two deadline-guarded tests in
`crates/hiroz-tests/tests/reentrant_lifecycle.rs`. With the fix reverted and
the tests kept, both fail:

  transition_callback_querying_own_state_does_not_deadlock
      ~/get_state did not answer while the transition callback was
      running - the state-machine mutex was held across the callback: Timeout(8s)
  failing_transition_callback_still_observes_intermediate_state   (same)

With the fix, both pass, and `lifecycle` stays at 30/30.
diff --git a/crates/hiroz-tests/tests/reentrant_lifecycle.rs b/crates/hiroz-tests/tests/reentrant_lifecycle.rs
@@ -0,0 +1,217 @@
+//! Re-entrancy audit for lifecycle transition callbacks.
+//!
+//! `ZLifecycleNode::trigger_transition` used to invoke the user's transition
+//! callback from inside `self.state_machine.lock().unwrap().trigger(..)`, i.e.
+//! with the state-machine mutex held for the whole duration of the callback.
+//!
+//! That mutex is not private to the transition: it is shared with the node's
+//! own `~/get_state`, `~/change_state` and `~/get_available_transitions` service
+//! handlers, and with `get_current_state()`. So while a transition callback ran,
+//! the node could not answer any question about its own state. A callback that
+//! asks — directly, or by waiting on anything that asks — waits forever.
+//!
+//! The fix splits `StateMachine::trigger` into `begin` (enter the intermediate
+//! "busy" state) and `finish` (apply the callback's verdict), so the node drops
+//! the guard while the callback runs. Observers keep seeing exactly what they
+//! saw before: the intermediate state (`configuring`, `activating`, …).
+//!
+//! Every scenario runs on a dedicated thread behind a hard deadline, so a
+//! re-entrancy deadlock fails the test instead of wedging the suite — the same
+//! shape as `reentrant_publish.rs` and `reentrant_service.rs`.
+
+mod common;
+
+use std::{
+    sync::{
+        Arc, Mutex,
+        atomic::{AtomicBool, Ordering},
+        mpsc,
+    },
+    thread,
+    time::Duration,
+};
+
+use common::{TestRouter, create_hiroz_context_with_endpoint};
+use hiroz::{
+    Builder,
+    lifecycle::{CallbackReturn, LifecycleState, ZLifecycleClient},
+};
+use serial_test::serial;
+
+/// Budget for one scenario. Generous relative to the work done — anything slower
+/// than this is a hang, not slowness.
+const SCENARIO_TIMEOUT: Duration = Duration::from_secs(45);
+
+/// How long the in-callback service call is allowed to take. Shorter than the
+/// scenario budget so a blocked handler surfaces as a failed assertion rather
+/// than as an unhelpful whole-scenario timeout.
+const CALL_TIMEOUT: Duration = Duration::from_secs(8);
+
+fn with_deadline(name: &'static str, scenario: impl FnOnce() + Send + 'static) {
+    let (tx, rx) = mpsc::channel();
+    thread::spawn(move || {
+        scenario();
+        let _ = tx.send(());
+    });
+    match rx.recv_timeout(SCENARIO_TIMEOUT) {
+        Ok(()) => {}
+        Err(mpsc::RecvTimeoutError::Disconnected) => {
+            panic!("{name}: scenario panicked — see the worker thread's panic above")
+        }
+        Err(mpsc::RecvTimeoutError::Timeout) => {
+            panic!("{name}: scenario did not finish within {SCENARIO_TIMEOUT:?} — deadlock")
+        }
+    }
+}
+
+/// Query `~/get_state` from a thread with its own runtime and return the answer.
+///
+/// The transition callback runs on the caller's thread, which may already be
+/// inside a runtime, so the query gets a plain std thread of its own — the same
+/// pattern `reentrant_service.rs` uses for a nested service call.
+fn get_state_blocking(client: Arc<ZLifecycleClient>) -> zenoh::Result<LifecycleState> {
+    thread::spawn(move || {
+        let rt = tokio::runtime::Builder::new_current_thread()
+            .enable_all()
+            .build()
+            .unwrap();
+        rt.block_on(async { client.get_state(CALL_TIMEOUT).await })
+    })
+    .join()
+    .expect("get_state worker panicked")
+}
+
+/// A transition callback that asks its own node what state it is in.
+///
+/// The `~/get_state` handler runs on a zenoh RX thread and takes the same
+/// state-machine mutex the transition holds. Under the old code it blocks until
+/// the transition completes — but the transition cannot complete, because its
+/// callback is waiting for that very reply. Deadlock, broken only by the client
+/// timeout, which then fails the assertion.
+///
+/// Introspecting your own lifecycle state during a transition is ordinary
+/// (rclcpp exposes `get_current_state()` for exactly this), and a lifecycle
+/// manager polling `~/get_state` while a node configures is even more ordinary.
+#[test]
+#[serial]
+fn transition_callback_querying_own_state_does_not_deadlock() {
+    with_deadline("lifecycle_transition_get_state", || {
+        const NODE_NAME: &str = "lc_reentrant_get_state";
+
+        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");
+
+        // The querying side lives in its own context, as a real lifecycle
+        // manager would.
+        let ctx_client = create_hiroz_context_with_endpoint(router.endpoint()).expect("client ctx");
+        let mgr_node = ctx_client
+            .create_node("lc_manager")
+            .build()
+            .expect("mgr node");
+
+        thread::sleep(Duration::from_millis(1000));
+        let client =
+            Arc::new(ZLifecycleClient::new(&mgr_node, NODE_NAME).expect("lifecycle client"));
+        thread::sleep(Duration::from_millis(1000));
+
+        let ran = Arc::new(AtomicBool::new(false));
+        let seen: Arc<Mutex<Option<zenoh::Result<LifecycleState>>>> = Arc::new(Mutex::new(None));
+
+        let ran_c = ran.clone();
+        let seen_c = seen.clone();
+        let client_c = client.clone();
+        lc_node.on_configure = Box::new(move |_prev| {
+            ran_c.store(true, Ordering::SeqCst);
+            // Re-entrant query of this node's own state, from inside its own
+            // transition callback.
+            *seen_c.lock().unwrap() = Some(get_state_blocking(client_c.clone()));
+            CallbackReturn::Success
+        });
+
+        let final_state = lc_node.configure().expect("configure");
+
+        assert!(ran.load(Ordering::SeqCst), "on_configure never ran");
+
+        let seen = seen.lock().unwrap().take().expect("no state recorded");
+        let seen = seen.expect(
+            "~/get_state did not answer while the transition callback was running — \
+             the state-machine mutex was held across the callback",
+        );
+        assert_eq!(
+            seen,
+            LifecycleState::Configuring,
+            "the callback must observe the intermediate transition state"
+        );
+        assert_eq!(final_state, LifecycleState::Inactive);
+    });
+}
+
+/// The same hazard on the failure path: a callback that returns `Failure` must
+/// still have been able to introspect, and the node must still land on the
+/// pre-transition state.
+///
+/// This pins the ordering the split introduced: `begin` publishes the
+/// intermediate state, `finish` applies the verdict *after* the callback
+/// returns. If the two halves were reordered, the observed state here would be
+/// `Unconfigured` rather than `Activating`.
+#[test]
+#[serial]
+fn failing_transition_callback_still_observes_intermediate_state() {
+    with_deadline("lifecycle_failing_transition_get_state", || {
+        const NODE_NAME: &str = "lc_reentrant_fail";
+
+        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");
+
+        let ctx_client = create_hiroz_context_with_endpoint(router.endpoint()).expect("client ctx");
+        let mgr_node = ctx_client
+            .create_node("lc_manager_fail")
+            .build()
+            .expect("mgr node");
+
+        thread::sleep(Duration::from_millis(1000));
+        let client =
+            Arc::new(ZLifecycleClient::new(&mgr_node, NODE_NAME).expect("lifecycle client"));
+        thread::sleep(Duration::from_millis(1000));
+
+        // Get to Inactive first, so `activate` is a legal transition.
+        assert_eq!(
+            lc_node.configure().expect("configure"),
+            LifecycleState::Inactive
+        );
+
+        let seen: Arc<Mutex<Option<zenoh::Result<LifecycleState>>>> = Arc::new(Mutex::new(None));
+        let seen_c = seen.clone();
+        let client_c = client.clone();
+        lc_node.on_activate = Box::new(move |_prev| {
+            *seen_c.lock().unwrap() = Some(get_state_blocking(client_c.clone()));
+            CallbackReturn::Failure
+        });
+
+        let final_state = lc_node.activate().expect("activate");
+
+        let seen = seen.lock().unwrap().take().expect("no state recorded");
+        let seen = seen.expect(
+            "~/get_state did not answer while the transition callback was running — \
+             the state-machine mutex was held across the callback",
+        );
+        assert_eq!(
+            seen,
+            LifecycleState::Activating,
+            "the callback must observe the intermediate transition state"
+        );
+        // Failure reverts to the start state.
+        assert_eq!(final_state, LifecycleState::Inactive);
+        assert_eq!(lc_node.get_current_state(), LifecycleState::Inactive);
+    });
+}
diff --git a/crates/hiroz/src/lifecycle/client.rs b/crates/hiroz/src/lifecycle/client.rs
@@ -173,6 +173,16 @@ fn state_from_lc(s: &LcState) -> LifecycleState {
         2 => LifecycleState::Inactive,
         3 => LifecycleState::Active,
         4 => LifecycleState::Finalized,
+        // Transition ("busy") states. A node genuinely reports these while a
+        // transition callback is running; mapping them onto `Unconfigured` told
+        // a lifecycle manager the node had reset itself, which is both wrong and
+        // indistinguishable from the real thing.
+        10 => LifecycleState::Configuring,
+        11 => LifecycleState::CleaningUp,
+        12 => LifecycleState::ShuttingDown,
+        13 => LifecycleState::Activating,
+        14 => LifecycleState::Deactivating,
+        15 => LifecycleState::ErrorProcessing,
         _ => LifecycleState::Unconfigured,
     }
 }
diff --git a/crates/hiroz/src/lifecycle/node.rs b/crates/hiroz/src/lifecycle/node.rs
@@ -129,44 +129,80 @@ impl ZLifecycleNode {
         let start = self.get_current_state();
         debug!(node=%self.inner.entity.name, ?transition, ?start, "triggering lifecycle transition");
 
-        let cb_result = {
-            let callback: &dyn Fn(State) -> CallbackReturn = match transition {
-                TransitionId::Configure => self.on_configure.as_ref(),
-                TransitionId::Activate => self.on_activate.as_ref(),
-                TransitionId::Deactivate => self.on_deactivate.as_ref(),
-                TransitionId::Cleanup => self.on_cleanup.as_ref(),
-                TransitionId::UnconfiguredShutdown
-                | TransitionId::InactiveShutdown
-                | TransitionId::ActiveShutdown => self.on_shutdown.as_ref(),
-            };
-            self.state_machine
-                .lock()
-                .unwrap()
-                .trigger(transition, callback)
+        let callback: &dyn Fn(State) -> CallbackReturn = match transition {
+            TransitionId::Configure => self.on_configure.as_ref(),
+            TransitionId::Activate => self.on_activate.as_ref(),
+            TransitionId::Deactivate => self.on_deactivate.as_ref(),
+            TransitionId::Cleanup => self.on_cleanup.as_ref(),
+            TransitionId::UnconfiguredShutdown
+            | TransitionId::InactiveShutdown
+            | TransitionId::ActiveShutdown => self.on_shutdown.as_ref(),
+        };
+
+        // The transition is driven in two locked steps with the user callback in
+        // between, *outside* the lock. `state_machine` is shared with the
+        // `~/get_state` / `~/change_state` service handlers and with
+        // `get_current_state`, so running the callback under the guard would
+        // deadlock any callback that inspects its own node.
+        //
+        // NOTE: the `begin` call is bound to its own `let` on purpose. Writing
+        // `match self.state_machine.lock().unwrap().begin(..) { .. }` keeps the
+        // temporary guard alive for the whole `match` body — which silently
+        // reintroduces exactly the deadlock this split exists to remove.
+        let begun = self.state_machine.lock().unwrap().begin(transition);
+        let cb_result = match begun {
+            Some(start_state) => {
+                // Lock released; observers see the intermediate ("busy") state.
+                let ret = callback(start_state);
+                self.state_machine
+                    .lock()
+                    .unwrap()
+                    .finish(transition, start_state, ret)
+            }
+            // Invalid transition from the current state: nothing changed.
+            None => self.get_current_state(),
         };
 
         let final_state = if cb_result == State::ErrorProcessing {
+            // Same split for the error path.
+            let ret = (self.on_error)(State::ErrorProcessing);
             self.state_machine
                 .lock()
                 .unwrap()
-                .trigger_error_processing(|prev| (self.on_error)(prev))
+                .finish_error_processing(ret)
         } else {
             cb_result
         };
 
-        // Bulk-activate / bulk-deactivate managed entities
-        match (start, final_state) {
-            (_, State::Active) if start != State::Active => {
-                for e in self.managed_entities.lock().unwrap().iter() {
+        // Bulk-activate / bulk-deactivate managed entities.
+        //
+        // Snapshot the list under the guard and notify after dropping it, the
+        // same collect-then-invoke shape used for event callbacks. Today every
+        // registered entity is a `ZLifecyclePublisher` created by
+        // `create_publisher`, whose `on_activate`/`on_deactivate` only flip an
+        // atomic and cannot re-enter — so this is preventive, not a live fix.
+        // It stops being preventive the moment `ManagedEntity` becomes
+        // registerable from outside this module, because arbitrary
+        // implementations would then run under a guard that `create_publisher`
+        // also takes.
+        let activate = match (start, final_state) {
+            (_, State::Active) if start != State::Active => Some(true),
+            (State::Active, _) if final_state != State::Active => Some(false),
+            _ => None,
+        };
+        if let Some(activate) = activate {
+            // The snapshot is its own `let` statement so the guard is dropped
+            // before the loop — writing the lock inline in the `for` header
+            // would hold it across every notification instead.
+            let entities: Vec<Arc<dyn ManagedEntity>> =
+                self.managed_entities.lock().unwrap().clone();
+            for e in entities {
+                if activate {
                     e.on_activate();
-                }
-            }
-            (State::Active, _) if final_state != State::Active => {
-                for e in self.managed_entities.lock().unwrap().iter() {
+                } else {
                     e.on_deactivate();
                 }
             }
-            _ => {}
         }
 
         // Publish transition event
diff --git a/crates/hiroz/src/lifecycle/state_machine.rs b/crates/hiroz/src/lifecycle/state_machine.rs
