Kill supervisor orphaned children - #49
Merged
Merged
Conversation
Children were spawned through `ros2 run`, which runs the node as a child of itself. Terminating the wrapper left the node alive, so every restart (and every shutdown) leaked a robot_state_publisher and a ros2_control_node, and the next start stacked a second controller_manager on the orphaned first. Two controller_managers claim the same hardware interfaces and command the same joints, so the robot follows whichever wrote last; /joint_states alternates between the two, which is what the panel showed. Resolve the executable and run it directly so a signal reaches the node, and refuse to start when a controller_manager that we do not own is already on the graph.
… the log COLLADA is UTF-8, but read_text() with no encoding uses the locale codec — cp1252 on a French Windows — which mangles accented content and rejects a handful of bytes outright, surfacing in the panel as "could not load any meshes". No shipped mesh trips it today; it is a portability trap, not the cause of a current failure. The supervisor also logged every child that it stopped on purpose as an error with 50 lines of replayed output, which buries the reports that matter. Mark children we are stopping and stay quiet about those. Cover node_argv with tests so nobody reintroduces the `ros2 run` wrapper.
_run_command read the child's stdout to EOF and only then called wait(timeout), so the timeout could not fire for the case it exists to cover: a child that hangs without exiting never closes the pipe and the read loop blocks forever. Both the 300s firmware build and the 120s picotool flash were unbounded in practice. Kill on a timer instead, which closes the pipe and ends the loop. The supervisor expanded the URDF through `ros2 run xacro`, whose wrapper has the same problem on a timeout; resolve xacro directly, keeping the PATH lookup as a fallback.
…ut to it ExecuteProcess(cmd=['ros2', 'launch', ...], shell=True) put five processes between launch and rosbridge on Windows: cmd.exe, the ros2 console-script shim, the nested launch, the libexec stub, then the node. Shutdown signalled only the outermost, so rosbridge survived and kept port 9090 bound; the next core then talked to a bridge attached to the previous graph. Including the launch file drops that to the stub and the node, both owned by the launch that started them. AnyLaunchDescriptionSource handles the .xml without pulling in launch_xml.
…nges ament_flake8 rejected web_ros_api.launch.py: the new AnyLaunchDescriptionSource import went after PythonLaunchDescriptionSource instead of before it, which failed the Linux and macOS colcon test step. The supervisor picked up two more import warnings for the same reason. PackageNotFoundError subclasses KeyError, so that import is redundant, and get_package_prefix is only needed inside node_argv.
Mael-RABOT
approved these changes
Sep 2, 2026
charlesmadjeri
approved these changes
Sep 2, 2026
Arcod7
added a commit
that referenced
this pull request
Sep 7, 2026
* fix(lucy_ros2_control): skip gtest build on macOS RoboStack Signed-off-by: Charles Madjeri <80175305+charlesmadjeri@users.noreply.github.com> * fix(ci): skip uncrustify/cppcheck lint on RoboStack macOS/Windows Signed-off-by: Charles Madjeri <80175305+charlesmadjeri@users.noreply.github.com> * evol(docs): clarify macOS/Windows test skips and Linux CI coverage Signed-off-by: Charles Madjeri <80175305+charlesmadjeri@users.noreply.github.com> * Macos hardware plugin link (#47) * Stop the hardware plugin loading rosidl Python bindings on macOS controller_manager could not load LucyHardwareLeftArm: Failed to load library liblucy_ros2_control.dylib … symbol not found in flat namespace '_PyExc_RuntimeError' so no hardware initialised, no controllers spawned, and nothing ever published /joint_states — the control panel had no positions to draw and no way to move the robot. ament_target_dependencies expands to each dependency's full ament link list, which includes every message package's __rosidl_generator_py library. Those are built to be loaded inside a Python interpreter: they carry undefined _Py* symbols and deliberately do not link libpython, because under `python3` the symbols are already in the process. This plugin uses no symbol from any of them, but they still landed in its load commands — 15 of them — and ros2_control_node is pure C++, so the flat-namespace lookup found nothing and pluginlib rejected the component. Linux binds these lazily and never hits it. Link the namespaced CMake targets instead, and add -dead_strip_dylibs on Apple so dylibs no symbol is used from are dropped before they can be loaded. The plugin's load commands go from 158 dylibs to 12, with zero generator_py. Verified: LibraryLoadException 0, all four controllers active (left_arm, right_arm, torso_head, joint_state_broadcaster), /joint_states has a publisher and streams live positions. * Emit a platform-neutral mock_sensor plugin filename The template hardcodes filename="libmock_sensor.so", but the plugin builds as libmock_sensor.dylib on macOS, so Gazebo cannot load it there. A bare "mock_sensor" lets Gazebo resolve the platform extension, which is the usual convention for gz plugin references. Without this the generator would overwrite the same fix applied to the checked in description/gazebo/gazebo.xacro of inmoov_urdf and thais_urdf on the next config regeneration. * fix(control_supervisor): surface child process output instead of swallowing it RSP, ros2_control_node and the spawners are started with stdout=PIPE and stderr=STDOUT, but nothing ever read those pipes: a child that failed left no trace, and one that outgrew the pipe buffer blocked forever. _track() drains every child and reports a non-zero exit. Children already log through ROS, so drained lines go to debug and the last 50 are replayed at error level only when a child exits badly. This is what hid ros2_control_node crashing on Windows: the stack came up with no controller_manager and no error anywhere. * Kill supervisor orphaned children (#49) * fix(supervisor): stop orphaning the control stack on restart Children were spawned through `ros2 run`, which runs the node as a child of itself. Terminating the wrapper left the node alive, so every restart (and every shutdown) leaked a robot_state_publisher and a ros2_control_node, and the next start stacked a second controller_manager on the orphaned first. Two controller_managers claim the same hardware interfaces and command the same joints, so the robot follows whichever wrote last; /joint_states alternates between the two, which is what the panel showed. Resolve the executable and run it directly so a signal reaches the node, and refuse to start when a controller_manager that we do not own is already on the graph. * fix(pipeline): read meshes as UTF-8; keep expected child exits out of the log COLLADA is UTF-8, but read_text() with no encoding uses the locale codec — cp1252 on a French Windows — which mangles accented content and rejects a handful of bytes outright, surfacing in the panel as "could not load any meshes". No shipped mesh trips it today; it is a portability trap, not the cause of a current failure. The supervisor also logged every child that it stopped on purpose as an error with 50 lines of replayed output, which buries the reports that matter. Mark children we are stopping and stay quiet about those. Cover node_argv with tests so nobody reintroduces the `ros2 run` wrapper. * evol(supervisor): trim the comments added with the orphan fix * fix(pipeline): arm the build/flash timeout before the streaming read _run_command read the child's stdout to EOF and only then called wait(timeout), so the timeout could not fire for the case it exists to cover: a child that hangs without exiting never closes the pipe and the read loop blocks forever. Both the 300s firmware build and the 120s picotool flash were unbounded in practice. Kill on a timer instead, which closes the pipe and ends the loop. The supervisor expanded the URDF through `ros2 run xacro`, whose wrapper has the same problem on a timeout; resolve xacro directly, keeping the PATH lookup as a fallback. * fix(bringup): include the rosbridge launch file instead of shelling out to it ExecuteProcess(cmd=['ros2', 'launch', ...], shell=True) put five processes between launch and rosbridge on Windows: cmd.exe, the ros2 console-script shim, the nested launch, the libexec stub, then the node. Shutdown signalled only the outermost, so rosbridge survived and kept port 9090 bound; the next core then talked to a bridge attached to the previous graph. Including the launch file drops that to the stub and the node, both owned by the launch that started them. AnyLaunchDescriptionSource handles the .xml without pulling in launch_xml. * fix(lint): restore import order broken by the rosbridge and xacro changes ament_flake8 rejected web_ros_api.launch.py: the new AnyLaunchDescriptionSource import went after PythonLaunchDescriptionSource instead of before it, which failed the Linux and macOS colcon test step. The supervisor picked up two more import warnings for the same reason. PackageNotFoundError subclasses KeyError, so that import is redundant, and get_package_prefix is only needed inside node_argv. * fix(bringup): give xacro a posix base_path so mesh URIs resolve on Windows (#50) base_path is pasted into a file:// URI in inmoov.urdf.xacro; Windows backslashes made that URI unresolvable and Gazebo rendered the robot with no geometry. * Faster control stack startup (#51) * fix(control-supervisor): wait on the graph instead of fixed sleeps Bringing the control stack up took 15-38s depending on luck. Most of it was waiting on timers rather than on work. The spawners were started 1s apart, before controller_manager could answer. They serialise on one lock file and hold it while waiting for /controller_manager/list_controllers, so the first one blocked for its full 20s lock attempt and the rest queued behind it: [WARN] spawner_right_arm_controller: Failed to acquire lock in 20 seconds. Attempt 1 of 5 Wait for the service before starting any spawner, and run them one at a time by waiting for each to exit. Measured on a headless bringup: all four controllers active at 17s, down from 38s when the lock contention hit. robot_state_publisher was followed by a 2s sleep; wait for a publisher on /robot_description instead. Its parameters arrive in a temp YAML written with delete=False and never removed -- a full URDF leaked per start, 24 files and 7.9MB on this machine -- so unlink it once the topic is up. lucy.launch.py deferred control.launch.py by 3s because the supervisor failed outright when config_pipeline_node had not yet generated the ros2_control xacro. The supervisor now waits for its input paths, so the timer is gone and the supervisor starts at 2.2s rather than 5.2s. DISCOVERY_SETTLE_S stays: the absence of a foreign controller_manager is not something that can be waited for. * feat(control-supervisor): record control-stack readiness in a file The launcher polls core's readiness, and asking the ROS graph is expensive: scripts/controllers_active.sh spends a Pixi activation and a throwaway rclpy node on every miss -- 1.3s when controller_manager answers, the full CALL_TIMEOUT of 12s when it does not. That cost is why the probe is cached for 8s, which in turn is why the TUI can sit on "Activating controllers" for several seconds after the robot is ready. Worse, the probe has to reach the graph at all. A probe scoped differently from the running stack -- a stack started before ROS_AUTOMATIC_DISCOVERY_RANGE was set to LOCALHOST, say -- sees nothing and reports a healthy robot as down, indefinitely. The supervisor already knows the answer: it starts the spawners and waits for each to exit. Record "<controller_manager pid> <controllers activated>" once they all succeed, and clear it whenever the children are terminated or this process exits. The pid makes a marker left by a killed supervisor fail a liveness check rather than vouch for a robot that is gone, and the count lets a reader answer any min_active threshold without asking the graph. Nothing is written when this supervisor does not own a controller_manager -- Gazebo runs it inside the simulator -- so that path keeps asking the graph. Measured against a real bringup, the readiness check drops from 1.73s to 7ms. --------- Signed-off-by: Charles Madjeri <80175305+charlesmadjeri@users.noreply.github.com> Co-authored-by: Charles Madjeri <80175305+charlesmadjeri@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OP#192