Skip to content

moveit2_server: bound the warm MoveItPy instance cache - #339

Closed
warun7 wants to merge 1 commit into
earthtojake:developfrom
warun7:fix/moveit2-cache-bounding
Closed

moveit2_server: bound the warm MoveItPy instance cache#339
warun7 wants to merge 1 commit into
earthtojake:developfrom
warun7:fix/moveit2-cache-bounding

Conversation

@warun7

@warun7 warun7 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

MoveItPyAdapter kept one warm MoveItPy instance per model forever - keyed by model asset hash, never evicted. Each instance is a ROS node plus a planning context, so serving many distinct robots crawled memory for the life of the server.

The cache is now bounded LRU:

  • default limit 5 warm instances, tunable via MOVEIT_PY_ADAPTER_CACHE (floored at 1)
  • a cache hit refreshes recency; the least-recently-used instance is shed when the bound is exceeded
  • eviction calls the instance's shutdown() best-effort rather than letting the native ROS node be silently collected; a shutdown failure is swallowed so eviction always proceeds
  • each model still pays its own first-touch cost - only the cross-model crawl is bounded

Verification

  • New MoveItPyCacheTests: LRU shedding order, recency refresh on hit, shutdown-failure tolerance (all without rclpy, matching the suite's existing style)
  • Full MoveIt2 server suite: 20 tests OK; scripts/test/test-python.sh all suites green

The adapter kept one warm MoveItPy instance per model forever -- a ROS node
plus planning context each, so serving many distinct robots crawled memory
for the life of the server. The cache is now bounded LRU (default 5,
MOVEIT_PY_ADAPTER_CACHE to change): a lookup refreshes recency, and shedding
the oldest instance shuts its node down best-effort rather than letting the
native state be silently collected.
@earthtojake

Copy link
Copy Markdown
Owner

@warun7 I've actually removed ros2 and moveit2 support on the release/0.5.0 branch, since i think this creates a bit too much bloat and probably isn't needed for the scope of the repo's skills. I'm ok to merge this in if you need it, but if not let's drop it.

@warun7

warun7 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

yeah if theres no support then it doesnt make sense
lets drop it

@warun7 warun7 closed this Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants