fix(brain): stop destroying ROS entities under spinning executors (skills-server cancel crash-loop) - #497
Conversation
Ending a skill (most visibly cancelling one from the Profiling page's run/Stop/run loop) destroyed nine subscriptions while executors were spinning the nodes that own them: RobotStateProvider.stop_subscriptions() on the skills server's MultiThreadedExecutor, and ManipulationInterface. stop() on its private single-threaded executor. Destroying a subscription the executor has already selected as ready races _take_subscription (InvalidHandle: 'destruction was requested'); the exception escapes spin(), main() skips teardown, and dying with live zenoh entities panics rmw_zenoh's Rust runtime -> SIGABRT -> ~2 min respawn crash-loop during which every /brain service times out. Reported by DavidDobas while testing PR #484, whose auto-stop makes every rollout end in a cancel. Reproduced in the CI image: create/destroy churn under 500 Hz traffic crashed within 6 cycles with the exact field signature. Fix follows the pattern manipulation_server.py already established for the same crash (2d188b6): create the subscriptions once, keep them for the node's lifetime, and gate the callbacks with an _active flag - the flag-gated variant survived 483 cycles clean. Cached state is still cleared on stop so skills never read stale data. Belt and braces: main() now catches exceptions escaping spin(), logs them FATAL, and exits through the ordered teardown, so any residual race of this class becomes a clean 2 s respawn instead of a SIGABRT cascade.
MobilityInterface destroyed and recreated its cmd_vel deadman timer at refresh rates (10-20/s) on nodes a live executor spins - on the skills server (MultiThreadedExecutor, timers touched from execute_callback worker threads) that is the same InvalidHandle wait-set race as the subscription teardown fixed in the previous commit. Replace the destroy/create cycle with a single persistent timer: retarget via timer_period_ns + reset() per command, cancel() in the callback for one-shot semantics. This also drops the cancelled-timer leak that the old destroy-based approach existed to avoid. Verified live on Humble under rmw_zenoh: 20 Hz refreshes suppress firing, exactly one stop fires per deadman window, and the timer retargets across durations. The remaining runtime destroy sites (CameraCapture.stop, PoseTracker.stop, BrainLifecycle.reactivate_brain) are safe: they run on brain_client_node, which is spun single-threaded via spin_once with WS messages handed to the spin thread before handlers run, so destroys execute between callbacks. Document that constraint at each site so a future move to a multi-threaded executor doesn't silently reintroduce the crash.
Greptile SummaryThis PR fixes a crash-loop in the skills-server caused by destroying ROS subscriptions and timers while their owning executors were still spinning them — a race that manifests as
Confidence Score: 5/5Safe to merge. The changes are correctly scoped to the race conditions identified in the root cause analysis, well-tested on the target hardware, and follow an established pattern already in the codebase. The core fix — never destroying subscriptions under a spinning executor, and parking the private executor between skills instead — is mechanically sound. The executor lifecycle in ManipulationInterface is correct: subscriptions survive across cycles, the old executor drops the node from its wait set during shutdown, and the new executor picks up existing subscriptions cleanly. The mobility timer change is verified on Humble. The skills_server.py fallback catch ensures any spin() escape goes through ordered teardown. No behavioral regressions identified. No files require special attention. The three comment-only files document existing constraints and need no further review. Important Files Changed
Reviews (3): Last reviewed commit: "fix(brain): disarm pending deadman stop ..." | Re-trigger Greptile |
…ation callbacks The always-alive subscriptions from the InvalidHandle fix cost ~51% of a Jetson core while idle (pre-fix baseline: 2-3%, measured on R7-27). The cost is not deserialization (raw=True subscriptions changed nothing) but rclpy executor dispatch at the ~630 msg/s the feeds add up to. - ManipulationInterface.start()/stop() now spin up / park its private executor. Subscriptions stay alive for the node's lifetime (never destroyed — same crash safety as before), but while parked, incoming messages just rotate in the bounded rmw queues at no Python cost. Idle CPU measured back down to 4%. - RobotStateProvider's five feeds move onto that private node so the same parked executor covers them (their lifecycle was already 1:1 with manipulation start/stop). - Gate the four manipulation callbacks with _active as the stop() docstring already claimed (the flag was set but never checked). Verified live on R7-27 (Humble/rmw_zenoh): 60 run->cancel cycles across head_emotion + wave, zero crashes, stable pid; behavior-goal delegation stays healthy after churn (8/8 wave goals accepted — the pre-fix destroy pattern deterministically broke zenoh action replies after ~40 cycles, manifesting as "Timeout waiting for behavior goal acceptance" with the replay still executing); turn_in_place reads odometry through the park/unpark cycle; full wave round trip succeeds.
A skill switching from timed to continuous motion previously still got cut short by the stop armed for the earlier timed command. Cancel the persistent stop timer when send_cmd_vel is called without a duration (cancel only disarms; it never destroys the handle, so the executor race this PR fixes stays out of reach).
…navigator leak) (#505) * fix(brain): destroy retired skill instances' ROS entities on reload (navigator leak) Every skill reload built each code skill twice (a throwaway instance in SkillLoader._get_name plus the kept one) and dropped the previous kept instance with no teardown. Dropped instances are cyclic garbage (Node -> subscription -> bound callback -> Node), so their ROS entities stayed in the zenoh graph until a rare gen-2 GC pass. For navigate_to_position that is 3 BasicNavigator nodes (~10 action clients each) per instance: the live box showed 8 duplicate subscriber sets on every */_action/feedback|status topic across /, /mapfree and /navigation after an 83-reload storm, ~20 MB RSS growth per reload, and 30-40 s reload latency. Fix, verified against the live graph with GC disabled (entity count now stays at exactly one set across reload cycles and drops to zero on final retire): - Skill.shutdown(): lifecycle hook for skill-owned ROS entities. Entities on the shared server node are deliberately left alone (see #497 -- no destroys under a spinning executor). - Nav2Controller/SimPathPlanningController.destroy(): destroy the navigator nodes, including assisted_teleop_client, which Humble's BasicNavigator.destroy_node() misses and whose live handle would keep the rcl node registered. - SkillLoader._get_name: shut the throwaway instance down. - SkillRepository: retire replaced/pruned instances on reload_all, reload_selective and _prune_stale_skills. - SkillsActionServer: disposal gate -- retired instances are destroyed immediately when idle, or deferred to execute_callback's finally while a skill is running (a mid-run reload may retire the running instance, whose entities its execute() is still spinning). * style: noqa B027 on Skill.shutdown — optional lifecycle hook, not abstract (matches inputs/types.py) * fix: don't let a failed assisted_teleop client destroy skip destroy_node (review)
…ify parse, drop dead aliases - shutdown() now flags a reloaded-out PickAnyObject instance so its leaked /pick_any_object/tuning subscription (undestroyable on the shared node, see #497) stops answering with stale params alongside the live instance - grasp verify parses Gemini's answer with startswith("NO") instead of a substring match that also hit CANNOT/NOT, which could report a false pick - tuning filter rejects JSON booleans (isinstance(True, int) is true) - remove back-compat aliases in skill_lib/arm.py: the module is new in this PR, nothing references them
…source lifecycle - detect_opponent_move: the Gemini client becomes @resource — built on the first chess run instead of at skill discovery, so boot no longer pays for a genai.Client on every robot. Calibration stays load-time on purpose (absent calibration must warn at discovery, not error). - pick_any_object: the Innate proxy client becomes @resource (lazy, was eager at load), and the tuning-panel topics become _DebugIO(SkillResource) — release() mutes instead of destroying, turning the #497 shared-node-entities constraint from a docstring into code. The hand-rolled _retired flag, _ensure_debug_io and the shutdown() override are gone; base Skill.shutdown() handles retirement.
Measured with pyright standard mode over workspace/innate_skills, skill_lib, brain_client/skills and innate: 40 errors -> 7, and all 7 are unresolvable-import noise on macOS (ROS-generated msgs, google.genai). No real type errors remain. Framework: - resource is Generic[T]: self.controller now types as its real class instead of Any (the annotation form already did; the decorator escape hatch now matches) - SkillResource declares __init__(self, skill) — the construction contract subclasses implement, and what makes T(skill) check - SkillReturn alias states execute()'s real contract (None | str | (message, status[, data])); all 23 shipped skills annotate it - wait_for is generic: wait_for(lambda: self.image) yields MainImage | None - self.skills is SkillInvoker | None (was inferred as None, so every self.skills.run(...) was an editor error) - descriptor _attr_name is str; vars(self) replaces self.__dict__ (MappingProxyType); SkillStorage/_send_feedback/set_feedback_callback typed - innate exports the lazy interfaces under TYPE_CHECKING so Mobility/ Manipulation/Head resolve for checkers as well as at runtime Real bug the checker found: ManipulationInterface.move_to_cartesian_pose / move_to_joint_positions annotate duration: int while the body does float(duration) and every caller passes fractional seconds — now float. Skills: - navigate_with_vision's ActionClient becomes a _NavigateClient SkillResource (drops the None-slot + lazy-if, release() documents the #497 shared-node rule) - detect_opponent_move helpers take Image | None, not str | None, so .jpeg checks; pick_any_object's _DebugIO asserts its node up front
Six findings from a recall-oriented review of the branch: - vision: clamp Gemini's normalized 0-1000 coords (_norm1k). They drift out of range, and an off-image/negative box reached seg_model's numpy slice and CamShift, raising cv2.error out past execute()'s handlers instead of taking the designed "lost track -> blind descent" fallback. - loader: a file defining two skills no longer loses both. Keep the one the filename names (the catalog id), drop the extras with an actionable error — rejecting the whole file vanished a previously-working skill because of a stray helper class. - camera_provider: refcount per feed, not just overall. A feed only a nested child declared (raw depth, ~600 KB/frame) streamed for the rest of the parent's run — exactly the cost start(feeds) exists to avoid. Dropping one parks the private spin thread first, since destroying entities under a spinning executor races it (#497); survivors miss at most one frame interval. - robot_state: the pre-run warmup now waits for every declared feed, not just required ones plus cameras. An optional/legacy declaration's first message is just as much in flight, so a skill reading self.odom in its opening lines saw None for one publish period and silently took its degraded path — pick's rotate_by fell back to open-loop with no log at all. The wait still ends as soon as the values land. Also gives moblib.rotate_by the open-loop warning drive already had. - types: reject `resource: SomeResource | None` at class definition. The optional flag was computed and then dropped, so the annotation built the same eager, never-None resource as the bare form and left the author's None guards as dead code. - pick_any_object: guidelines text moves into the class docstring (the API's own rule) and the override goes. It was also building the debug_io resource as a side effect, so every catalog metadata refresh created a publisher + subscription on the shared node — which _DebugIO deliberately never destroys. debug_io now builds on the first _dbg. Also applies ruff format to pick_any_object.py, which was left unformatted by eb4852f (one list reflow) — the format gate is red on the branch without it. Tests: two new cases in test_ambient_robot_state.py (loader keeps the filename skill; optional resource annotation rejected). 27 pass.
Skill instances now live for exactly one run instead of being long-lived singletons, which deletes a large amount of cross-run hygiene machinery. - Catalog holds classes + metadata harvested once at discovery, never live instances; a reload is just an entry swap with nothing to retire. - Each run gets a throwaway node; the run's finally releases the instance's @resource objects then destroys the node wholesale (the #497-safe teardown), so anything a skill or resource created on self.node dies with it. - Construction happens only inside the claimed execution slot (or from a running parent's invoker) — never while the robot is otherwise busy. - cancel routes to the live per-run instance via _active_code_skill, guarded by goal-handle identity so a stale cancel can't hit a fresh run. - Deleted: retired-instance disposal/gating, clear_robot_state(), the _begin_run latch-clear + _clear_cancel_hooks, all_code_skills(), and the duplicated "disappeared during reload" re-fetch branches. - Collapsed behavior-goal tracking (3 id-keyed collections) into one owned slot keyed by the owning goal handle. - Replaced PrimitiveStub with plain metadata dicts in SkillRegistry. - Physical skills: one read/validate helper for load+reload, a PhysicalSkillEntry dataclass, and publish from the stored episode_count instead of re-reading it from disk. - @resource teardown is now a single mechanism: a generator factory releases after its yield (dropping @x.teardown and the close/destroy/shutdown naming convention). Per-run wiring folded into _instantiate_for_run. say() now briefly waits for the TTS engine to match its fresh per-run publisher so a run's first utterance isn't dropped.
…omments - execute() is introspected once, at discovery: the published schema, the int->float input coercion and the invoker's input validation all read CodeSkillEntry (inputs/float_params/accepts_extra_inputs). Drops both runtime inspect passes. - Single _FeedSpec table: annotation types, typo hints, failure labels, camera keys and warmup graces all derive from one row per feed. - Shared _Injected descriptor base behind RobotState/Interface. - Result plumbing collapsed (_FINALIZE table + _abort_result); robot-state injection unified over _state_getters with typed camera accessors; registration builds the SkillInfo field list once. - Aggressive comment pass: docstrings cut to the non-obvious facts; safety notes (#497, cancel races, hardware limits) kept as one-liners. - Rename get_required_robot_states -> declared_robot_state_types; typed Image.from_jpeg as a generic classmethod.
…source lifecycle - detect_opponent_move: the Gemini client becomes @resource — built on the first chess run instead of at skill discovery, so boot no longer pays for a genai.Client on every robot. Calibration stays load-time on purpose (absent calibration must warn at discovery, not error). - pick_any_object: the Innate proxy client becomes @resource (lazy, was eager at load), and the tuning-panel topics become _DebugIO(SkillResource) — release() mutes instead of destroying, turning the #497 shared-node-entities constraint from a docstring into code. The hand-rolled _retired flag, _ensure_debug_io and the shutdown() override are gone; base Skill.shutdown() handles retirement.
Measured with pyright standard mode over workspace/innate_skills, skill_lib, brain_client/skills and innate: 40 errors -> 7, and all 7 are unresolvable-import noise on macOS (ROS-generated msgs, google.genai). No real type errors remain. Framework: - resource is Generic[T]: self.controller now types as its real class instead of Any (the annotation form already did; the decorator escape hatch now matches) - SkillResource declares __init__(self, skill) — the construction contract subclasses implement, and what makes T(skill) check - SkillReturn alias states execute()'s real contract (None | str | (message, status[, data])); all 23 shipped skills annotate it - wait_for is generic: wait_for(lambda: self.image) yields MainImage | None - self.skills is SkillInvoker | None (was inferred as None, so every self.skills.run(...) was an editor error) - descriptor _attr_name is str; vars(self) replaces self.__dict__ (MappingProxyType); SkillStorage/_send_feedback/set_feedback_callback typed - innate exports the lazy interfaces under TYPE_CHECKING so Mobility/ Manipulation/Head resolve for checkers as well as at runtime Real bug the checker found: ManipulationInterface.move_to_cartesian_pose / move_to_joint_positions annotate duration: int while the body does float(duration) and every caller passes fractional seconds — now float. Skills: - navigate_with_vision's ActionClient becomes a _NavigateClient SkillResource (drops the None-slot + lazy-if, release() documents the #497 shared-node rule) - detect_opponent_move helpers take Image | None, not str | None, so .jpeg checks; pick_any_object's _DebugIO asserts its node up front
Six findings from a recall-oriented review of the branch: - vision: clamp Gemini's normalized 0-1000 coords (_norm1k). They drift out of range, and an off-image/negative box reached seg_model's numpy slice and CamShift, raising cv2.error out past execute()'s handlers instead of taking the designed "lost track -> blind descent" fallback. - loader: a file defining two skills no longer loses both. Keep the one the filename names (the catalog id), drop the extras with an actionable error — rejecting the whole file vanished a previously-working skill because of a stray helper class. - camera_provider: refcount per feed, not just overall. A feed only a nested child declared (raw depth, ~600 KB/frame) streamed for the rest of the parent's run — exactly the cost start(feeds) exists to avoid. Dropping one parks the private spin thread first, since destroying entities under a spinning executor races it (#497); survivors miss at most one frame interval. - robot_state: the pre-run warmup now waits for every declared feed, not just required ones plus cameras. An optional/legacy declaration's first message is just as much in flight, so a skill reading self.odom in its opening lines saw None for one publish period and silently took its degraded path — pick's rotate_by fell back to open-loop with no log at all. The wait still ends as soon as the values land. Also gives moblib.rotate_by the open-loop warning drive already had. - types: reject `resource: SomeResource | None` at class definition. The optional flag was computed and then dropped, so the annotation built the same eager, never-None resource as the bare form and left the author's None guards as dead code. - pick_any_object: guidelines text moves into the class docstring (the API's own rule) and the override goes. It was also building the debug_io resource as a side effect, so every catalog metadata refresh created a publisher + subscription on the shared node — which _DebugIO deliberately never destroys. debug_io now builds on the first _dbg. Also applies ruff format to pick_any_object.py, which was left unformatted by eb4852f (one list reflow) — the format gate is red on the branch without it. Tests: two new cases in test_ambient_robot_state.py (loader keeps the filename skill; optional resource annotation rejected). 27 pass.
Skill instances now live for exactly one run instead of being long-lived singletons, which deletes a large amount of cross-run hygiene machinery. - Catalog holds classes + metadata harvested once at discovery, never live instances; a reload is just an entry swap with nothing to retire. - Each run gets a throwaway node; the run's finally releases the instance's @resource objects then destroys the node wholesale (the #497-safe teardown), so anything a skill or resource created on self.node dies with it. - Construction happens only inside the claimed execution slot (or from a running parent's invoker) — never while the robot is otherwise busy. - cancel routes to the live per-run instance via _active_code_skill, guarded by goal-handle identity so a stale cancel can't hit a fresh run. - Deleted: retired-instance disposal/gating, clear_robot_state(), the _begin_run latch-clear + _clear_cancel_hooks, all_code_skills(), and the duplicated "disappeared during reload" re-fetch branches. - Collapsed behavior-goal tracking (3 id-keyed collections) into one owned slot keyed by the owning goal handle. - Replaced PrimitiveStub with plain metadata dicts in SkillRegistry. - Physical skills: one read/validate helper for load+reload, a PhysicalSkillEntry dataclass, and publish from the stored episode_count instead of re-reading it from disk. - @resource teardown is now a single mechanism: a generator factory releases after its yield (dropping @x.teardown and the close/destroy/shutdown naming convention). Per-run wiring folded into _instantiate_for_run. say() now briefly waits for the TTS engine to match its fresh per-run publisher so a run's first utterance isn't dropped.
…omments - execute() is introspected once, at discovery: the published schema, the int->float input coercion and the invoker's input validation all read CodeSkillEntry (inputs/float_params/accepts_extra_inputs). Drops both runtime inspect passes. - Single _FeedSpec table: annotation types, typo hints, failure labels, camera keys and warmup graces all derive from one row per feed. - Shared _Injected descriptor base behind RobotState/Interface. - Result plumbing collapsed (_FINALIZE table + _abort_result); robot-state injection unified over _state_getters with typed camera accessors; registration builds the SkillInfo field list once. - Aggressive comment pass: docstrings cut to the non-obvious facts; safety notes (#497, cancel races, hardware limits) kept as one-liners. - Rename get_required_robot_states -> declared_robot_state_types; typed Image.from_jpeg as a generic classmethod.
Problem
While testing #484 on-robot, @DavidDobas hit a reproducible skills-server crash-loop immediately after skill cancellations (report): rclpy
InvalidHandle→ rmw_zenoh Rust panic → SIGABRT, respawned by launch, with every/brainservice timing out for ~2 minutes. The Profiling page's run → Stop → run-again rollout loop (and #484's auto-stop, which ends every rollout in a cancel) hits it constantly.Root cause
The trigger is our own teardown, not just the upstream races. At the end of every skill,
execute_callback'sfinallydestroyed nine subscriptions while executors were spinning the nodes that own them:RobotStateProvider.stop_subscriptions()— five subs (/odom,/map,/joint_states, head, battery) on the skills-server node, which spins on aMultiThreadedExecutorManipulationInterface.stop()— four subs on its private node, spun by its own executor threadDestroying a subscription the executor has already selected as ready races
Executor._take_subscription(with sub.handle:→InvalidHandle: cannot use Destroyable because destruction was requested). The exception re-raises in the spin thread (_spin_once_impl→future.result()),main()only caughtKeyboardInterrupt, so the process unwound pastdestroy()/shutdown()— and exiting with live zenoh entities is what panics rmw_zenoh's Rust runtime into SIGABRT.manipulation_server.pyalready diagnosed and fixed this exact race for its own sensor subscriptions (2d188b6, "deliberately NEVER destroyed"); the skills server kept the racy pattern.Repro (CI image, Humble + rmw_zenoh): a worker thread cycling create/destroy of 5 subs under 500 Hz traffic on a
MultiThreadedExecutornode crashed with the exact field signature within 6 cycles. The flag-gated pattern below survived 483 cycles / 60k messages clean.Fix
Same pattern as
manipulation_server.py: create subscriptions once, keep them for the node's lifetime, gate the callbacks with an_activeflag (robot_state.py,manipulation.py). Cached state is still cleared on stop, so skills never read stale data; idle cost is bounded by message deserialization.Also in this PR:
skills_server.pymain()— belt-and-braces guard: exceptions escapingspin()are logged FATAL and the node exits through the ordered teardown, so any residual race of this class becomes a clean 2 s respawn instead of a SIGABRT cascade (this is also the upstream-recommended mitigation, see Exception occurs when iterating waitables in rclpy executors ros2/rclpy#1385).mobility.py— the cmd_vel deadman destroyed/recreated its stop timer at 10–20/s on the same spinning nodes (same race class, reachable from skills driving the base). Replaced with one persistent timer retargeted viatimer_period_ns+reset(), cancelled in its callback for one-shot semantics — which also removes the cancelled-timer leak the destroy approach existed to avoid. Verified live on Humble/rmw_zenoh: refreshes suppress firing, exactly one stop per deadman window, retargets across durations.camera.py/pose_tracking.py/lifecycle.py— their runtime destroys are safe (brain_client_node is spun single-threaded viaspin_once, WS handlers run on the spin thread, so destroys happen between callbacks); added a comment at each site stating that constraint so a future executor change doesn't silently reintroduce the crash.Upstream context
InvalidHandleexecutor races are known and fixed only in rolling (Lock the goal-handle / result-future registry in ActionServer ros2/rclpy#1668 registry locking, Exception occurs when iterating waitables in rclpy executors ros2/rclpy#1385 waitable iteration) — no Humble backports.ros-humble-rmw-zenoh-cpp≤ 0.1.8 (zenoh 1.5.0), which lacks the zenoh 1.8.0 "undeclare blocks until callbacks finish" fix (rmw_zenohhumblesource branch since 2026-04, never binary-released). Building that branch from source would add fleet-wide hardening independent of this fix.result_timeout(900 s) after goal creation, not completion (Goal status corruption when goal takes longer to finishes than ActionServer's result_timeout ros2/rcl#970), so cancelling a goal that ran > 15 min destroys its handle at the cancel instant — a ceiling on safe skill runtimes under Humble.Verification
InvalidHandlewithin 6 cycles; fixed pattern: clean over 483 cycles / 60k msgs (CI image, zenoh router, 500 Hz × 5 topics)ruff check/ruff format/pre-commit(.config/pre-commit-config.yaml): pass on all 7 filestest_fake_cloud_selftest.py,test_backwards_compat.py): 19 passedRepro: run a learned skill from the Profiling page, hit Stop, immediately run again, repeat — previously crash-looped the skills server within a few cycles; with this fix the loop runs clean.