Hold ToolManager instances weakly in the class registry

[Lokkhita]

Pre-PR Checklist

• This PR is a bug fix, not a new feature or enhancement.
  Summary
  ToolManager.instances held a strong reference to every manager ever created (one per LLMAgent), so none could be garbage-collected. This PR makes the registry weak and keeps add_tool_to_all() safe under concurrent construction, which a bare WeakSet does not.
  Bug / Issue
  Fixes ToolManager instances are retained after they are no longer referenced #336.

Measured on main: after building and discarding 20 models × 25 agents, 500 managers were still registered (agents and models themselves were freed). Memory grows without bound across long runs and batch sweeps, and every add_tool_to_all() call also iterates all dead managers.
Implementation

• instances is now a weakref.WeakSet, so managers are released as soon as the application drops them.
• A class-level threading.Lock guards add() and a snapshot of the set in add_tool_to_all(). Registration then runs on the snapshot, outside the lock.

Why not only a WeakSet (as in #337): iterating a WeakSet while another thread adds to it raises RuntimeError: Set changed size during iteration. The old list never raised this, so a bare WeakSet swaps a leak for a crash. This is reachable when agents are created during a threaded step (step_agents_multithreaded) while a tool is broadcast.

Why the lock is released before register(): threading.Lock is not re-entrant, so holding it while registering would deadlock if registration ever constructs a manager.

Alternatives considered:

• List of weakref.ref with removal callbacks: callbacks fire during GC, possibly mid-iteration, and mutating a list during iteration silently skips entries.
• Removing the registry: a breaking API change, out of scope for a bug fix.
  Testing
  New regression tests in tests/test_tools/test_tool_manager.py:

Test	main	bare WeakSet	this PR
test_unreferenced_manager_is_garbage_collected	❌	✅	✅
test_add_tool_to_all_skips_collected_managers	❌	✅	✅
test_add_tool_to_all_tolerates_manager_created_mid_broadcast	✅	❌ RuntimeError	✅

• Thread stress (4 threads constructing managers + 3000 concurrent broadcasts): bare WeakSet raises RuntimeError; this PR runs clean.
• Leak measurement (20 × 25 agents discarded): 500 → 0 managers retained.
• Full suite: 1555 passed. pre-commit run passes.
  Additional Notes
• instances keeps the operations used in the codebase (len, in, iteration, .clear()). .append() and indexing are no longer available; nothing in the repo uses them.
• A WeakSet is unordered. Broadcast order has no effect because each register() is independent.
• A manager constructed during a broadcast is not guaranteed to receive that broadcast's tool (point-in-time semantics).
• Relation to Fix ToolManager instance retention #337: it covers the leak with a bare WeakSet; this PR additionally covers the iteration race that change introduces. Happy to consolidate with Fix ToolManager instance retention #337 in whatever way maintainers prefer.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: b3f46dc3-c716-491b-ad14-b79aa9b80f60

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Lokkhita]

Closing in favour of #337, which was opened first and, since its Sep 22 update,also handles the mid-iteration RuntimeError (retry-snapshot).The lock-based variant here stays available if maintainers prefer deterministic serialisation over retry; I've offered it as a follow-up on #337.