fix(brain): destroy retired skill instances' ROS entities on reload (navigator leak) - #505
Merged
Merged
Conversation
…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).
Contributor
Greptile SummaryThis PR adds deterministic cleanup for skill reloads. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "fix: don't let a failed assisted_teleop ..." | Re-trigger Greptile |
…tract (matches inputs/types.py)
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
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.
Problem
Every skill reload leaks ROS entities. Two compounding paths, neither with teardown:
SkillLoader._get_name(), which builds a throwaway instance of each skill class just to read.name; the catalog then builds the kept one. Fornavigate_to_positioneach instance is 3 BasicNavigator nodes (~10 action clients each). The sim skill's throwaway adds another navigator even in real-robot mode (it's only dropped after discovery, in_apply_sim_swap).reload_all()/reload_selective()overwrite the catalog dicts, orphaning old instances. Orphans are cyclic garbage (Node → subscription → bound callback → Node), so their zenoh graph entities stay registered until a gen-2 GC pass — which an idle process may not run for hours.Measured on the live robot (mars-the-44th, 2026-07-07)
/,/mapfree,/navigationafter an 83-reload storm (a client hammered/brain/reloadat ~100 Hz for 30 min — separate bug, tracked separately)./brain/reload_primitives: +2 entity sets in/mapfree&/navigation, +3 in root (sim throwaway), +19.5 MB RSS, 39.5 s duration.gc.collect()releases all of them — nothing hard-pins them, they're just uncollected cyclic garbage.Fix
Skill.shutdown()lifecycle hook (no-op default). Deliberately leaves shared-server-node entities alone — destroying entities under a spinning executor is unsafe (fix(brain): stop destroying ROS entities under spinning executors (skills-server cancel crash-loop) #497).Nav2Controller.destroy()/SimPathPlanningController.destroy()— destroy the navigator nodes, includingassisted_teleop_client, which Humble'sBasicNavigator.destroy_node()misses and whose live handle keeps the rcl node registered (destroy_when_not_in_use).SkillLoader._get_name— shuts the throwaway instance down.SkillRepository— retires replaced/pruned instances onreload_all,reload_selective, and_prune_stale_skills.SkillsActionServer— disposal gate: retired instances are destroyed immediately when idle, or deferred toexecute_callback'sfinallywhile a skill runs. A mid-run reload can retire the currently executing instance, whose entities itsexecute()is still spinning; destroying those live would recreate the fix(brain): stop destroying ROS entities under spinning executors (skills-server cancel crash-loop) #497 crash class. Navigator nodes are never attached to any executor when no skill is executing, so idle-time destruction is exactly as safe as the GC destruction that already happens today — just deterministic.Verification
gc.disable(), three simulated reload cycles against the live graph hold at exactly 1 entity set (previously +2/cycle) and drop to 0 on final retire, including/assisted_teleop/_action/*and the sim skill's nodes.test_backwards_compat.py, 12 loader/discovery tests) passes with the patched loader — the newshutdown()call in_get_nameis exercised by every discovery test.py_compileclean on all six files; 120-col ruff limit respected.Note for deploy:
brain_clientneeds a rebuild + skills-server restart; the twoworkspace/innate_skills/files hot-reload on their own afterward.