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

[YuanYuYuan]

Part of #282 — the defect class, the shared fix shape and the merge order are stated there.

Role in #282

Instance fix. Independent: targets main, consumes nothing from #255, collides with nothing else in flight. Can merge in any order.

Issue

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 for the whole duration of a transition callback the node could not answer any question about itself — for anybody, including a lifecycle manager in another process. A callback that asked never completed at all.

Verified both directions, 2026-08-05 — stated with the commit each direction was measured on, because they differ:

direction	result	measured on
fix present	4 passed	the current tip, including the Unknown change
the three lifecycle/ production files reverted, tests kept	all four fail	the branch before the rebase onto main

The revert run has not been repeated since the rebase. Nothing in the rebase or the two commits after it touched the transition path, so it should still hold — but it is not re-measured, and this note is here rather than a claim that it is.

The failure path is the reproduction: the inner ~/get_state query does not return within its 8 s call timeout, and the test converts that into a named panic rather than an unhelpful whole-scenario deadline.

What this PR does

Split StateMachine::trigger into begin and finish. begin validates and enters the intermediate ("busy") state; the callback runs with no guard held; finish applies its verdict. trigger is kept as a wrapper for callers that own the machine outright, and the error path gets the same split.

This is the family's acq · callout · rel → acq · rel · callout rewrite, expressed as a state-machine split rather than a clone-and-drop, because the callback's verdict has to be applied afterwards.

sequenceDiagram
    autonumber
    participant M as Manager<br/>(other process)
    participant T as Node thread
    participant SM as state_machine<br/>mutex
    participant CB as user on_configure

    T->>SM: lock() → begin(Configure)
    activate SM
    SM-->>T: start_state, state := Configuring
    deactivate SM
    Note over T,SM: guard released before the callout

    T->>CB: callback(start_state)
    activate CB
    CB->>SM: get_current_state() → lock()
    SM-->>CB: Configuring ✓
    M->>T: ~/get_state
    T-->>M: Configuring ✓
    CB-->>T: CallbackReturn::Success
    deactivate CB

    T->>SM: lock() → finish(verdict)

Loading

Compare against the diagram in #252: there the activation bar on state_machine spans the callback; here it does not. That is the whole change.

Two consequences worth reviewing rather than skimming:

• The begin result is bound to its own let deliberately. Writing match self.state_machine.lock().unwrap().begin(..) { .. } would keep the temporary guard alive for the whole match body and silently reintroduce the exact deadlock this split removes. There is a comment saying so at the site.
• A panicking callback is now caught, the machine settled, and the panic re-raised. Moving the callout out of the lock removed an accidental safety net: previously an unwind passed through a live guard and poisoned the mutex, so the next access failed loudly. Without the guard, an unwind would leave current at Configuring/Activating forever with an unpoisoned mutex — every later begin returns None and ~/get_state answers configuring for the life of the process. A silent wedge. Hence catch_unwind + resume_unwind.

state_from_lc in lifecycle/client.rs stopped mapping every transition state onto Unconfigured. This is load-bearing for the tests, not cosmetic: without it the intermediate state is unobservable over the wire, so a test cannot tell "answered correctly" from "answered Unconfigured".

State gains Unknown = 0. Having fixed ids 10–15, the catch-all still folded PRIMARY_STATE_UNKNOWN — and any unrecognised id — into Unconfigured: the same conflation, one value narrower. It matters because unknown and unconfigured are different instructions to a manager; unconfigured is actionable, and the correct response is to call configure() on a node that never claimed to need it.

It is reachable. hiroz can never emit 0 — a live machine starts at Unconfigured and only ever assigns primary or transition states, so current.id() never yields Unknown — but this decoder reads other nodes — including rclcpp ones, which do have UNKNOWN in their vocabulary — and a default-constructed State message also carries id = 0, so a response that failed to populate decoded as a confident "unconfigured".

The enum now mirrors lifecycle_msgs/msg/State exactly. Only label() needed a new arm: shutdown_for and available_transitions already had catch-alls, and is_primary lists the primary states explicitly, so Unknown is correctly excluded.

Breaking changes

#	What changes	Who is affected	Before → After	Action
1	ZLifecycleClient reports transition states	callers reading state during a transition	Unconfigured → Configuring / Activating / …	stop treating Unconfigured as "transition in progress" — that reading was this client's bug
2	LifecycleState gains Unknown = 0	downstream exhaustive match on it	compiles → fails to compile	add an Unknown arm
3	Panicking callback settles the machine	anyone depending on a panic leaving it mid-transition	wedged state → terminal verdict, panic re-raised	none

Not affected: non-hiroz observers — an rclcpp lifecycle manager, ros2 lifecycle get. The node's published id was always correct; only hiroz's own client mis-decoded it.

⚠️ Row 3 depends on unwinding. Under [profile.opt] (panic = "abort") a panicking callback aborts the process, as before. dev, test and release unwind and are covered.

Nothing removed — StateMachine::trigger and trigger_error_processing are retained as wrappers.

Known, not fixed here

ZLifecycleClient::shutdown() picks a transition with match … { Unconfigured => …, Active => …, _ => InactiveShutdown }, so an Unknown node now gets an InactiveShutdown guess. That is pre-existing behaviour — the transition states already fell through the same arm — and it is the same "guess rather than admit ignorance" shape this PR fixes one layer down. Out of scope here; worth its own look.

Checklist

• Added/updated tests/documentation (if applicable)
• cargo clippy -p hiroz --all-targets --features jazzy -- -D warnings clean, and reentrant_lifecycle 4/4, on the current tip
• ./scripts/check-local.sh — passed on the original commits, not re-run after the rebase and the two commits since. CI is covering it.

[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) {