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

[YuanYuYuan]

Description

Fixes #252

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.

Branches off main and stands alone.

Key Changes

• crates/hiroz/src/lifecycle/state_machine.rs — 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.

  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, and the code says so.

• crates/hiroz/src/lifecycle/client.rs — fix state_from_lc, the client-side parser, which mapped every transition-state id onto Unconfigured via its catch-all arm. Ids 10–15 (Configuring, CleaningUp, ShuttingDown, Activating, Deactivating, ErrorProcessing) now map to themselves. This is also what makes the intermediate state observable to the new test.

• crates/hiroz/src/lifecycle/node.rs — managed entities get collect-then-invoke as well: trigger_transition called e.on_activate() / e.on_deactivate() while iterating under managed_entities.lock(), which create_publisher also takes. This one is not reachable today and is fixed preventively: the only way to register an entity is create_publisher, so every element is a ZLifecyclePublisher whose activate/deactivate only flip an atomic. It ships without a test on purpose — a test 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 have to re-derive it.

• Splitting trigger means a panicking callback could strand the machine in its intermediate state, with no lock held and no verdict ever applied — Configuring forever, and every subsequent transition rejected. Both callback sites are therefore wrapped in catch_unwind: the machine is finished with Failure (or finish_error_processing(Error) on the error path) and the payload is re-raised with resume_unwind, so a panicking callback still fails exactly as loudly as before. See "Breaking Changes" for the one profile where this guard does not apply.

What fails without this

Detector proven in both directions. With the fix reverted and the two deadline-guarded tests in crates/hiroz-tests/tests/reentrant_lifecycle.rs 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.

No regression: lifecycle 30/30, hiroz --lib 282/282.

Breaking Changes

One observable change, and it is the intended fix — but it is narrower than "the node's reporting changed". The node always reported its genuine intermediate state over the wire. The id it publishes was correct before this PR; what was wrong was hiroz's own ZLifecycleClient, which discarded ids 10–15 in a catch-all and surfaced Unconfigured to its caller.

So:

• Code using ZLifecycleClient now sees Configuring / Activating / … where it previously saw Unconfigured. Anything reading Unconfigured as "transition in progress" was reading a bug in this client.
• Non-hiroz observers (an rclcpp lifecycle manager, ros2 lifecycle get) are unaffected — they parse the wire value themselves and always saw the correct state.

Panic semantics, stated because they are new: a panic in a transition or on_error callback is now caught, the state machine is advanced to a terminal verdict, and the panic is re-raised. The net effect for a caller is unchanged (the panic still propagates), but the machine is no longer left mid-transition. This guard relies on unwinding, so it does not apply under the [profile.opt] profile, which sets panic = "abort" — there a panicking callback aborts the process, as it did before. dev, test and release all unwind and are covered.

No API is removed. StateMachine::trigger and trigger_error_processing are retained as wrappers.

Notes

cargo doc is red on main today for unrelated reasons (three unresolved intra-doc links in error.rs); pr/0-rustdoc-links clears it. This branch adds no doc links.

Checklist

• Ran ./scripts/check-local.sh successfully
• Added/updated tests/documentation (if applicable)

[Copilot]

Pull request overview

Fixes lifecycle callback deadlocks by releasing the state-machine lock during callbacks and exposing accurate intermediate states.

Changes:

• Splits transitions into locked begin/finish phases.
• Corrects lifecycle client transition-state mappings.
• Adds re-entrancy regression tests and lock-free managed-entity notifications.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
state_machine.rs	Adds split transition APIs.
node.rs	Runs callbacks outside mutex guards.
client.rs	Maps intermediate state IDs correctly.
reentrant_lifecycle.rs	Tests callback state queries.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

crates/hiroz-tests/tests/reentrant_lifecycle.rs:16

• This says observers retain the old behavior, but previously the mutex prevented observers from reading any state until the callback completed, and the client also mapped busy-state IDs to Unconfigured. The PR intentionally changes observers to see the intermediate state, so describe that new behavior rather than claiming it was already observable.

//! the guard while the callback runs. Observers keep seeing exactly what they
//! saw before: the intermediate state (`configuring`, `activating`, …).

crates/hiroz/src/lifecycle/node.rs:212

• If on_error panics after an error from an Active-state transition, this branch moves the machine to Finalized and immediately unwinds past the managed-entity synchronization below. The node then reports Finalized while its lifecycle publishers remain activated and can still publish. Deactivate the managed entities before resuming the panic (ideally through the same helper used by normal completion) so the externally visible state and entity gating remain consistent.

                    self.state_machine
                        .lock()
                        .unwrap()
                        .finish_error_processing(CallbackReturn::Error);
                    resume_unwind(payload);

crates/hiroz/src/lifecycle/state_machine.rs:216

• finish is a new public safe API that can bypass every transition invariant: for example, calling StateMachine::new().finish(Activate, Inactive, Success) moves directly from Unconfigured to Active, because neither the matching begin nor the expected intermediate state is validated. The documented caller obligation does not prevent accidental state corruption. Keep the split methods crate-private if they are only for ZLifecycleNode, or make begin return an opaque token that finish consumes and validates; apply the same restriction to finish_error_processing.

    /// Second half of [`Self::trigger`]: apply the user callback's verdict.
    ///
    /// `start_state` must be the value returned by the matching [`Self::begin`].
    pub fn finish(

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

crates/hiroz/src/lifecycle/node.rs:205

• The panic-recovery branch for on_error is not covered. If this catch_unwind or its settling call regresses, the node remains silently stuck in ErrorProcessing; the new test only checks an on_error callback that returns normally, while the normal transition callback has a dedicated panic test. Please add the equivalent panicking-on_error test and assert both panic propagation and the documented settled state (Finalized).

            let ret = match catch_unwind(cb) {